-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Improve sourcemaps of this
during async transformation
#15370
base: main
Are you sure you want to change the base?
Conversation
To be honest I'm not sure if |
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/53804/ |
this
during async transformationthis
during async transformation
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.
Do we still need the tests in packages/babel-traverse/test/fixtures/conversion
?
Maybe @jridgewell knows :) |
Makes sense! There seems to be a bug with this, currently only class methods are working. |
Honestly, no, none of these are necessary. Names are useful for things that were identifiers in the source code, not for things that become identifiers in the output. |
Yes, I tried it using this PR and the problem still persists. |
I think we will also need some special handling in js-debug; |
e14593f
to
e8611c6
Compare
Well, it's not a bug, it's because the test suite runner is broken, after rebasing everything works fine. In addition, I found a related PR a few years ago through git history. It seems that the Firefox debugger used to have special handling for I don't know if there is a better way. |
@liuxingbaoyu Do you have any estimation when will this fix be merged to main? |
@liuxingbaoyu Do you know when will this PR be merged? |
@Omcsesz Unfortunately this PR didn't get approved and I'm not sure if it will be merged. |
From my personal perspective, I completely agree that this is not something in the source map specification, but since the debugger authors of VS Code are also willing to support this use case, I don't mind merging it. What do you think? |
this
during async transformationthis
during async transformation
I think that you should merge it. |
No, in Babel a PR must be approved by two other reviewers before being merged. But inspired by #15921, we may be able to solve the most common |
I would be ok with merging this -- we are working on standardizing better source maps (https://github.com/tc39/source-map-rfc/), so it's important that the spec follows what's actually needed and not the other way around :) Also, what I learned so far is that nobody knows what |
@nicolo-ribaudo Good to hear. :) Is there any progress on this topic on Github Issues? |
sourcemap
CI failure needs to wait for #15361 to be merged.