-
-
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
Add support for helper dependencies #6254
Conversation
packages/babel-helpers/src/index.js
Outdated
throw child.buildCodeFrameError("Helpers may import anything."); | ||
const name = child.node.source.value; | ||
if (!helpers[name]) { | ||
throw child.buildCodeFrameError(`Unknown helper ${name}`); |
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.
This actually throws Cannot read property 'file' of undefined
, but it is not directly related to this PR (also the others throw child.buildCodeFrameError
in this file throw that error). I will fix it separately.
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/5122/ |
const { nodes, globals } = getHelper(name, uid, ownBindingNames); | ||
const { nodes, globals } = getHelper( | ||
name, | ||
name => this.addHelper(name), |
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.
I try to avoid using sideeffectful functions like addHelper
when writing utils that focus building and retrieving data like getHelper
since get*
as a name doesn't really make you expect the function to have sideeffects.
An alternate approach that comes to mind here would be that getHelpers
could accept a callback like this that just returns the name of the helper, if it exists, or null. Then getHelper
could be adjusted to return multiple helpers, and include the ones that don't exist yet in that list.
Thoughts?
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.
Actually getHelper
doesn't have side effects: the loadDependency
parameter is just "a function which, given an helper name, returns a node which references the helper": the side effects are in that that callback, but I think that it is easily recognizeable because its defined inside the addHelper
function (which has side effects).
For example, in babel-core/src/tools/babel-external-helpers.js
it is used in a side effects free way.
Maybe I should rename loadDependency
to something like getDependencyRef
.
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.
I was being grumpy about it because it technically means that the ownBindingNames
passed into the getHelper
will be outdated since it is calculated before the addHelper
call for the dependencies. If the dependencies happened to name something that conflicted with the helper that depends on it, they'd potentially conflict.
That said, it's probably an edge case that isn't worth worrying about.
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.
I fixed that possible bug by lazy-computing ownBindingNames
(so that they are retrieved after the dependencies are added).
packages/babel-helpers/src/index.js
Outdated
|
||
traverse(file, { | ||
ImportDeclaration(child) { | ||
throw child.buildCodeFrameError("Helpers may import anything."); |
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.
Ugh I typo everything huh? :P
helpers.list.forEach(function(name) { | ||
if (whitelist && whitelist.indexOf(name) < 0) return; | ||
|
||
const { nodes } = helpers.get( | ||
name, | ||
t.memberExpression(namespace, t.identifier(name)), | ||
getHelperReference, |
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.
We'll also need logic like this in babel-runtime/scripts/build-dist.js
so babel-runtime
has the correct imports and exports to link the files together.
packages/babel-helpers/src/index.js
Outdated
|
||
if (!binding) globals.add(name); | ||
if (name in dependencies) { | ||
importBindingsReferences.push(makePath(child)); |
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.
This may not reference the same identifier, need to check binding.
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.
Done
packages/babel-helpers/src/index.js
Outdated
@@ -136,9 +162,19 @@ function permuteHelperAST(file, metadata, id, localBindings) { | |||
toRename[exportName] = id.name; | |||
} | |||
|
|||
const dependenciesRefs = {}; | |||
for (const bindnig in dependencies) { |
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.
Typo.
0ac2a9b
to
1593c43
Compare
f30285b
to
70c6567
Compare
if (!binding) { | ||
globals.add(name); | ||
} else if (dependencies.has(binding.identifier)) { | ||
importBindingsReferences.push(makePath(child)); |
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.
What's the motivation for putting this here instead of doing it above where you have dependencies.set(bindingIdentifier, name)
?
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.
When I do dependencies.set(bindingIdentifier, name)
I don't have access to the NodePath of the reference, I can only use the path of the identifier inside the import statement.
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.
Ahh of course, I just missed that they were different nodes.
packages/babel-helpers/src/index.js
Outdated
@@ -170,6 +208,11 @@ function permuteHelperAST(file, metadata, id, localBindings) { | |||
path.scope.rename(name, toRename[name]); | |||
}); | |||
|
|||
for (const path of imps) path.remove(); | |||
for (const path of impsBindingRefs) { | |||
path.replaceWith(dependenciesRefs[path.node.name]); |
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.
We should wrap this in a t.cloneDeep()
call so we're not reusing the nodes.
|
||
// This is not garanteed to be 100% unique, but I hope | ||
// no one will ever name a babel helper like "_$_$_$_foo_12"! | ||
let i = 0; |
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.
Does having this increment like this mean that if we run the tests in a different order, we'll end up with different IDs?
I also have mixed feelings about these not deleting the helpers they add after the test has completed.
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.
Does having this increment like this mean that if we run the tests in a different order, we'll end up with different IDs?
Yes, but it isn't observable in expected.js
because the only part of the ID which changes is a number and thus it is stripped away by .generateUidIdentifier
. Anyway, I will make the helper name deterministic by using the test name instead of the i
variable.
I also have mixed feelings about these not deleting the helpers they add after the test has completed.
I could delete them after each fixture, but I would need to implement a custom fixtures runner or mock babel.transform
to remove them after the transformation is finished. I can't do it in a Program#exit
visitor because it isn't called if the test throws.
I don't think not sharing state between tests is worth this additional complexity, since the test helpers' names are unique and thus they can't conflict.
If you think there is a simpler way to remove the registered helpers, please let me know.
They are used like this: helpers.main = defineHelper(` import dep from "dependency"; export default function main() { return dependency; } `); helpers.dependency = defineHelper(`ì export default function dep() { return 0; } `);
70c6567
to
6d0517e
Compare
Amazing work @nicolo-ribaudo! Can we make a new PR to document this in the readme? I know there's not much in |
This commit updates the `getDependency` callback passed to `helpers.get()`. That callback only used to return an Identifier or MemberExpression which represents the helper; now it can return both the reference and the code of the dependency. Now babel-runtime/scripts/build-dist.js can easily replace `import foo from "foo"` with `var foo = require("foo")`. As a side effect of this change, helpers dependencies are directly added to the helper ast instead of the file's, making the call to `getHelper` inside `File#addHelper` actually pure, as requested in the review of the original helper dependencies PR [1] [1]: babel#6254 (comment)
This commit updates the `getDependency` callback passed to `helpers.get()`. That callback only used to return an Identifier or MemberExpression which represents the helper; now it can return both the reference and the code of the dependency. Now babel-runtime/scripts/build-dist.js can easily replace `import foo from "foo"` with `var foo = require("foo")`. As a side effect of this change, helpers dependencies are directly added to the helper ast instead of the file's, making the call to `getHelper` inside `File#addHelper` actually pure, as requested in the review of the original helper dependencies PR [1] [1]: babel#6254 (comment)
I had to add some tests even if the
babel-helpers
package didn't have them because without them I couldn't manage to make everything work 😅@peey To integrate this in #6107, I think you just have to remove everything that comes from #6058 and update the helpers to use
import
instead ofbabelHelpers.*
.