Skip to content

fix(destructuring): preserve loc of original declaration in output #13173

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 20, 2021
Merged

fix(destructuring): preserve loc of original declaration in output #13173

merged 1 commit into from
Apr 20, 2021

Conversation

motiz88
Copy link
Contributor

@motiz88 motiz88 commented Apr 19, 2021

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

When transforming a VariableDeclaration with the destructuring plugin, the declaration node currently loses its source location. The resulting source map does not allow the user to set a breakpoint on the declaration line. This can be seen in this example on CodeSandbox: https://codesandbox.io/s/eager-mcclintock-8ozxx

Screen.Recording.2021-04-19.at.18.11.23.mov

This PR fixes this case, which according to my local testing restores the ability to set the breakpoint as expected.

@babel-bot
Copy link
Collaborator

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

@hzoo
Copy link
Member

hzoo commented Apr 19, 2021

Good catch! nodes.push(t.inherits(destructuring.buildVariableAssignment creates a new t.expressionStatement or t.variableDeclaration which has no loc data even though inherits does copy it. I guess it'll always be undefined then?

outside of this PR, this is similar to what I've been working on with https://github.com/hzoo/babel-explorer, which is trying to do this generally. If this is affecting sourcemaps, in some sense we should attempt to look for all the places where the the original node is replaced/wrap and pass down the loc?

@nicolo-ribaudo nicolo-ribaudo added area: sourcemaps PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Apr 19, 2021
@motiz88
Copy link
Contributor Author

motiz88 commented Apr 20, 2021

I guess it'll always be undefined then?

@hzoo: Yeah, I'm only checking for an existing node.loc as a form of future-proofing in case we ever update DestructuringTransformer to emit more fine-grained locations.

@motiz88
Copy link
Contributor Author

motiz88 commented Apr 20, 2021

Does anyone have an idea about what's causing the CI failures? Seems like some random browserslist message is breaking some snapshots:

Browserslist: caniuse-lite is outdated. Please run:
npx browserslist@latest --update-db

Why you should do it regularly:
https://github.com/browserslist/browserslist#browsers-data-updating

@nicolo-ribaudo
Copy link
Member

Don't worry, it's fixed on main

nicolo-ribaudo
nicolo-ribaudo approved these changes Apr 20, 2021
View reviewed changes
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@JLHwung JLHwung merged commit 691c468 into babel:main Apr 20, 2021
@motiz88 motiz88 deleted the destructuring-decl-preserve-loc branch June 1, 2021 13:49
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Sep 1, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@JLHwung JLHwung JLHwung approved these changes

@nicolo-ribaudo nicolo-ribaudo nicolo-ribaudo approved these changes

Assignees
No one assigned
Labels
area: sourcemaps outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Bug Fix 🐛 A type of pull request used for our changelog categories
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@motiz88 @babel-bot @hzoo @nicolo-ribaudo @JLHwung