-
Notifications
You must be signed in to change notification settings - Fork 83
Convert JavaScript uncaught exception stack traces to Dart stacks #1603
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
Thank you Jason! It would be great to add a test so we don't introduce regressions in the future. |
Added a test |
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.
Thanks Jason! Left some comments to cover more cases.
There are a few things we need to do:
- address CR comments
- update dwds version to
14.0.2-dev
inchangelog.md
and pubspec.yaml (14.0.1
is in the making right now, so changing to14.0.2-dev
would be the easiest to resolve conflicts) - add a description of the fix to the
changelog.md
- run
dart run build_runner build
- commit and submit your changes
Thanks Jason! Let me know if you would like to me to take over at any point if it gets too complicated. |
Added handling for the VSCode case, and a check in an existing test for stack trace mapping on an exception pause event. PTAL |
0ff5051
to
fbaee5f
Compare
Thanks Jason, this is fantastic! Merging with master should resolve the test failures (you might need to update the version to 14.0.3-dev and rerun |
I rebased this on master and it's passing CI now. However, when I run Should I add the build_runner output to this PR, or can the PR be landed as is? Or do I need to do something else (such as running build_runner with a different version of the Dart SDK)? |
I am not sure what's happening, could be that your dart version is not the same as Elliott's, or some packages are outdated. I'll try to fix that part and append to your PR. |
@jason-simmons I built your PR and it seems to not generate anything bad, just some changes in the compiled |
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.
LGTM! Thank you Jason for your contribution!
I am going to merge this, thank you Jason! |
Fixes: #1265
Fixes: flutter/flutter#101888