-
Notifications
You must be signed in to change notification settings - Fork 2
Add TypeScript proxy to support JrS debugging w/ Chrome DevTools #4
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
Add TypeScript proxy to support JrS debugging w/ Chrome DevTools #4
Conversation
@martijnthe you could check my attempt to merge the Also, Travis failed I think because we haven't done |
I had added |
@knightburton we've moved over the PR from jerryscript-project/jerryscript#2205 to here. Can you take another review pass? I've asked @akosthekiss to add you to the team. Thanks. |
@grgustaf actually, there's more missing:
|
e0b0fb1
to
86d39b4
Compare
Yep, missed those files - just updated with the additions. |
86d39b4
to
5fb883c
Compare
LGTM |
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.
@martijnthe I've ran through the changes again.
src/lib/debugger-client.ts
Outdated
this.delegate.onMessage(new Uint8Array(data)); | ||
} | ||
|
||
send(data: ArrayBuffer) { |
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.
Maybe I am wrong, however I think this data
parameter should be Uint8Array
.
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.
Yeah, you're right, thanks for catching.
I'm a bit surprised the TypeScript compiler didn't throw an error about this.
@grgustaf will you fix it up?
Thanks yeah I'll be back tomorrow and will take care of that, sorry for the delay. |
Basic debugging features of step over, step in , step out, continue, stop, add/remove breakpoints, provide backtrace at breakpoint, and evaluate watch expressions are all working. A few low-hanging features still missing: - passthrough of output to CDT console - reporting exceptions - verbosity control Other features of CDT will require enhancement to JrS debugger, e.g.: - scope chain - inspectable eval output (e.g. JSON, not just strings) - memory analysis - profiling Significant level of unit testing provided via jest. Thanks to Martijn The <martijn.the@intel.com> for initial TS code skeleton, some unit tests, 'step out' feature, and tons of review! JerryScript-DCO-1.0-Signed-off-by: Geoff Gustafson geoff@linux.intel.com
5fb883c
to
1836ff1
Compare
Fixed that line, thanks... |
LGTM, thanks. @knightburton OK now? |
@martijnthe @grgustaf Yes, all good! :) LGTM |
@zherczeg @akosthekiss @yichoi Do you have an interest in reviewing this? |
@martijnthe I've skimmed through the patch, and as much as an expert I am with TS, the patch LGTM |
You can setup your own reviewing policies for this project. The only requirement is apache 2.0 license and DCO for clear reusability. |
OK, thanks... |
Basic debugging features of step over, step in , step out, continue, stop,
add/remove breakpoints, provide backtrace at breakpoint, and evaluate watch
expressions are all working.
A few low-hanging features still missing:
Other features of CDT will require enhancement to JrS debugger, e.g.:
Significant level of unit testing provided via jest.
Thanks to Martijn The martijn.the@intel.com for initial TS code skeleton,
some unit tests, 'step out' feature, and tons of review!
JerryScript-DCO-1.0-Signed-off-by: Geoff Gustafson geoff@linux.intel.com