Improve sourcemaps of this during async transformation

[liuxingbaoyu]

Q	A
Fixed Issues?	Fixes #15362
Patch: Bug Fix?	√
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass?	√
Documentation PR Link
Any Dependency Changes?
License	MIT

sourcemap

CI failure needs to wait for #15361 to be merged.

[liuxingbaoyu]

To be honest I'm not sure if names in the source map has such a purpose. 😕

[babel-bot]

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/53804/

[nicolo-ribaudo]

Do we still need the tests in packages/babel-traverse/test/fixtures/conversion?

[nicolo-ribaudo]

To be honest I'm not sure if names in the source map has such a purpose.

Maybe @jridgewell knows :)

[liuxingbaoyu]

Do we still need the tests in packages/babel-traverse/test/fixtures/conversion?

Makes sense! There seems to be a bug with this, currently only class methods are working.

[jridgewell]

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.

[liuxingbaoyu]

Yes, I tried it using this PR and the problem still persists.
@connor4312 Do you have any ideas?
this-super-arguments.zip

[connor4312]

I think we will also need some special handling in js-debug; this is given on the callframe and so takes a different code path to the Variables view than ordinary variables on the scopeChain. But the sourcemap in your zip file looks good, so if Babel decides to implement that then I can also make the corresponding changes in js-debug.

[liuxingbaoyu]

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.
#7312

It seems that the Firefox debugger used to have special handling for this, but obviously it will parse the source file for AST-level analysis, and js-debug does not do such a thing.

I don't know if there is a better way.
Personally I don't mind having this PR merged, even though it might be a bit out of the general use of source maps, there should be no downside after all.
What do you think?

[Omcsesz]

@liuxingbaoyu Do you have any estimation when will this fix be merged to main?

[Omcsesz]

@liuxingbaoyu Do you know when will this PR be merged?

[liuxingbaoyu]

@Omcsesz Unfortunately this PR didn't get approved and I'm not sure if it will be merged.

[liuxingbaoyu]

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?

[Omcsesz]

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?

I think that you should merge it.

[liuxingbaoyu]

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 this problem in another way.

[nicolo-ribaudo]

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 names is actually for. We are working on better alternative, but so far this is what we have.

[Omcsesz]

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 names is actually for. We are working on better alternative, but so far this is what we have.

@nicolo-ribaudo Good to hear. :) Is there any progress on this topic on Github Issues?