Skip to content
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

Create File class for babel helpers #10575

Merged
merged 1 commit into from Mar 17, 2020
Merged

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Oct 18, 2019

Q                       A
Fixed Issues? Fixes #8641, fixes #9496
Patch: Bug Fix? Yes
Tests Added + Pass? Yes
Any Dependency Changes? No
License MIT

In this PR we create a File instance for each helper snippets, by doing so we can ensure other visitors will have access to .hub when visiting the inserted nodes from helpers.

@JLHwung JLHwung added area: helpers PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Oct 18, 2019
const ast = t.file(helper.ast());
return new File(
{
filename: `babel-helper://${name}`,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

babel-helper:// is used to denote that this file is a babel helper snippet thus does not physically exist.

@@ -17,5 +17,8 @@
},
"devDependencies": {
"@babel/helper-plugin-test-runner": "^7.0.0"
},
"peerDependencies": {
"@babel/core": "^7.0.0-0"
Copy link
Member

Choose a reason for hiding this comment

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

Adding (or updating) a peer dependency is a technically a breaking change. (semver/semver#502)

Maybe we could pass File as a parameter to loadHelper (from get and ensure), and only instantiate it if it is present?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good to know about that! Though I am not sure if, practically, there is anyone using @babel/helpers 7 with babel-core 6.

I end up revising helpers.ensure to accept extra fileClass because get interface is complicated enough.

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.

Dependency injection feels hacky, at least in this case, but probably the only @babel/helper direct users are us 😛

* add test

* fix: pass File to helper traverser

* pass babel.File to helpers.ensure
@nicolo-ribaudo nicolo-ribaudo merged commit 4bf36e6 into babel:master Mar 17, 2020
@nicolo-ribaudo nicolo-ribaudo deleted the finding-hub branch March 17, 2020 08:58
@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 Jun 17, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: helpers 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
2 participants