Add support for helper dependencies by nicolo-ribaudo · Pull Request #6254 · babel/babel

nicolo-ribaudo · 2017-09-15T20:39:46Z

Q	A
Fixed Issues	Fixes #6030, closes #6058
Patch: Bug Fix?	no
Major: Breaking Change?	no
Minor: New Feature?	yes
Tests Added/Pass?	yes
Spec Compliancy?	no
License	MIT
Doc PR	no
Any Dependency Changes?	no

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 of babelHelpers.*.

nicolo-ribaudo · 2017-09-15T20:48:15Z

packages/babel-helpers/src/index.js

-      throw child.buildCodeFrameError("Helpers may import anything.");
+      const name = child.node.source.value;
+      if (!helpers[name]) {
+        throw child.buildCodeFrameError(`Unknown helper ${name}`);


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.

babel-bot · 2017-09-15T20:50:11Z

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

loganfsmyth · 2017-09-15T20:51:51Z

packages/babel-core/src/transformation/file/index.js

-    const { nodes, globals } = getHelper(name, uid, ownBindingNames);
+    const { nodes, globals } = getHelper(
+      name,
+      name => this.addHelper(name),


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?

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.

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.

I fixed that possible bug by lazy-computing ownBindingNames (so that they are retrieved after the dependencies are added).

loganfsmyth · 2017-09-15T20:52:16Z

packages/babel-helpers/src/index.js


  traverse(file, {
    ImportDeclaration(child) {
-      throw child.buildCodeFrameError("Helpers may import anything.");


Ugh I typo everything huh? :P

loganfsmyth · 2017-09-15T21:27:57Z

packages/babel-core/src/tools/build-external-helpers.js

    const { nodes } = helpers.get(
      name,
-      t.memberExpression(namespace, t.identifier(name)),
+      getHelperReference,


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.

jridgewell · 2017-09-15T23:25:59Z

packages/babel-helpers/src/index.js

-
-      if (!binding) globals.add(name);
+      if (name in dependencies) {
+        importBindingsReferences.push(makePath(child));


This may not reference the same identifier, need to check binding.

jridgewell · 2017-09-15T23:26:15Z

packages/babel-helpers/src/index.js

  }

+  const dependenciesRefs = {};
+  for (const bindnig in dependencies) {


loganfsmyth · 2017-09-26T23:58:32Z

packages/babel-helpers/src/index.js

+      if (!binding) {
+        globals.add(name);
+      } else if (dependencies.has(binding.identifier)) {
+        importBindingsReferences.push(makePath(child));


What's the motivation for putting this here instead of doing it above where you have dependencies.set(bindingIdentifier, name)?

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.

Ahh of course, I just missed that they were different nodes.

loganfsmyth · 2017-09-27T00:00:25Z

packages/babel-helpers/src/index.js


+      for (const path of imps) path.remove();
+      for (const path of impsBindingRefs) {
+        path.replaceWith(dependenciesRefs[path.node.name]);


We should wrap this in a t.cloneDeep() call so we're not reusing the nodes.

loganfsmyth · 2017-09-27T00:16:39Z

packages/babel-helpers/test/helpers/define-helper.js

+
+// 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;


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.

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; } `);

hzoo · 2017-10-02T18:04:40Z

Amazing work @nicolo-ribaudo! Can we make a new PR to document this in the readme?

I know there's not much in babel-helpers readme anyway 😛

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)

nicolo-ribaudo commented Sep 15, 2017

View reviewed changes

loganfsmyth reviewed Sep 15, 2017

View reviewed changes

jridgewell reviewed Sep 15, 2017

View reviewed changes

peey mentioned this pull request Sep 16, 2017

Decorators 2 Transform [WIP] #6107

Closed

nicolo-ribaudo force-pushed the helper-import branch 2 times, most recently from 0ac2a9b to 1593c43 Compare September 17, 2017 14:56

nicolo-ribaudo added the PR: Internal 🏠 A type of pull request used for our changelog categories label Sep 17, 2017

nicolo-ribaudo force-pushed the helper-import branch from f30285b to 70c6567 Compare September 17, 2017 21:10

jridgewell approved these changes Sep 17, 2017

View reviewed changes

loganfsmyth reviewed Sep 26, 2017

View reviewed changes

loganfsmyth reviewed Sep 27, 2017

View reviewed changes

Add support for helpers dependencies.

6d0517e

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; } `);

nicolo-ribaudo force-pushed the helper-import branch from 70c6567 to 6d0517e Compare October 1, 2017 16:59

nicolo-ribaudo added 2 commits October 1, 2017 19:26

Clone import references

8ce44e6

Don't make test helpers name depend on the order the tests are run

8b25b62

loganfsmyth merged commit 977c722 into babel:master Oct 2, 2017

nicolo-ribaudo deleted the helper-import branch October 2, 2017 17:18

hzoo mentioned this pull request Oct 2, 2017

Document babel-helpers + new dependency functionality #6363

Closed

nicolo-ribaudo mentioned this pull request Oct 12, 2017

Fix helper dependencies in babel runtime #6379

Merged

lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 5, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019

Uh oh!

Conversation

nicolo-ribaudo commented Sep 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

babel-bot commented Sep 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolo-ribaudo Sep 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hzoo commented Oct 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

babel-bot commented Sep 15, 2017 •

edited

Loading

nicolo-ribaudo Sep 15, 2017 •

edited

Loading