-
Notifications
You must be signed in to change notification settings - Fork 510
Add TypeScript, catch bugs #256
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
render() { | ||
let line = parseInt(this.props.line); | ||
let onclick = null, | ||
let onclick = () => {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug #2: React event handlers shouldn't be null.
cls = ""; | ||
if (!this.props.file || !line) { | ||
line = ""; | ||
line = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug #3: line
is a number, not a string.
gdbgui/src/js/Links.tsx
Outdated
@@ -1,14 +1,22 @@ | |||
import Actions from "./Actions.js"; | |||
import React from "react"; | |||
import CopyToClipboard from "./CopyToClipboard.jsx"; | |||
import MemoryLink from "./MemoryLink.jsx" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug #1: It looks like we forgot to import MemoryLink
before?
Thank you for this! It's amazing. A couple things:
Is this expected? |
Hey @cs01! I just came back from vacation.
That's definitely longer than expected. Will take a look in the next 1-2 weeks.
Looks like I misconfigured something in Webpack. Will take a look at that one too, probably a 1-liner. |
@cs01 Updated! |
Thanks! Will take a look when I get the time. |
I tried it out. It builds with no errors, and finds type errors while it's watching with The build time went from 5814ms to ~30 for intial build, but incremental builds are only ~1 second, so I can live with that. It looks like the source maps were removed, was there a reason for that? I would prefer to keep them if possible. |
Looking around some more, and it seems like the watcher is a little flaky. have you encountered this? I'm not sure if it's something wrong with my setup or not. If I run
but when I change a file, nothing happens. I saw this earlier, but killed the |
Thanks again @bcherny! |
This PR:
Tests pass, but I'd love help testing this end to end. It's likely that this doesn't work at runtime yet, and will require a little more tweaking.