feat: option to hoist nested methods referencing this (`methods: tr…

…ue`) See README.md for documentation. This feature is blocked on the following Babel PRs/issues: * babel/babel#4500 * babel/babylon#121 * babel/babel#4337 * babel/babel#4230 (partially)
motiz88 · Sep 11, 2016 · d6372bf · d6372bf
1 parent 0818f42
commit d6372bf
Show file tree

Hide file tree

Showing 19 changed files with 279 additions and 23 deletions.
diff --git a/README.md b/README.md
@@ -10,7 +10,9 @@
 Babel plugin to hoist nested functions to the outermost scope possible without changing their
 contract.
 
-## Example
+## Examples
+
+### Example 1 - basic hoisting
 
 **In**
 
@@ -32,6 +34,36 @@ function renderApp () {
 }
 ```
 
+### Example 2 - nested method hoisting
+
+To enable this transformation, pass the `methods: true` option to the plugin (see below).
+The output code depends on the ES2015 `Symbol` feature and the stage 2 class properties proposal.
+You will _most likely_ want to run `babel-plugin-transform-class-properties` after `transform-hoist-nested-function`.
+
+**In**
+
+```js
+class Foo {
+  bar () {
+    return () => this;
+  }
+}
+```
+
+**Out**
+
+```js
+const _hoistedMethod = new Symbol("_hoistedMethod"),
+
+class Foo {
+  [_hoistedMethod] = () => this;
+
+  bar() {
+    return this[_hoistedMethod];
+  }
+}
+```
+
 ## Motivation
 
 Patterns like [React "render callbacks"]
@@ -69,7 +101,7 @@ factory() === factory(); // ⬅ value depends on whether foo() is hoisted
 ```
 
 That last expression evaluates to `false` in plain JavaScript, but is `true` if `foo()` has been
-hoisted. 
+hoisted.
 
 More fundamentally, **references to hoisted inner functions are allowed to escape their enclosing
 scopes**. You should determine whether this is appropriate for your code before using this plugin.
@@ -96,10 +128,22 @@ $ npm install --save-dev babel-plugin-transform-hoist-nested-functions
 
 **.babelrc**
 
-```json
+```js
+// without options
 {
   "plugins": ["transform-hoist-nested-functions"]
 }
+
+// with options
+// NOTE: transform-class-properties is required in order to run the code
+{
+  "plugins": [
+    ["transform-hoist-nested-functions", {
+      "methods": true
+    }],
+    "transform-class-properties"
+  ]
+}
 ```
 
 ### Via CLI
@@ -126,7 +170,7 @@ cd babel-plugin-transform-hoist-nested-functions
 npm install
 # ... hackity hack hack ...
 npm run test:local # Including tests (mocha), code coverage (nyc), code style (eslint), type checks
-                   # (flow) and benchmarks.  
+                   # (flow) and benchmarks.
 ```
 
 See package.json for more dev scripts you can use.

diff --git a/src/index.js b/src/index.js
@@ -4,6 +4,7 @@ import type { NodePath, Scope } from 'babel-traverse';
 
 class InnerScopeVisitor {
   referencedScopes: Scope[];
+  thisReferencedScopes: Scope[];
 
   ReferencedIdentifier = (path: NodePath) => {
     const binding = path.scope.getBinding(path.node.name);
@@ -20,11 +21,10 @@ class InnerScopeVisitor {
     let {scope} = path;
     while (scope && (scope = scope.getFunctionParent())) { // eslint-disable-line no-cond-assign
       if (!scope.path.isArrowFunctionExpression()) {
-        // istanbul ignore next: could be initialized elsewhere
-        if (!this.referencedScopes) {
-          this.referencedScopes = [];
+        if (!this.thisReferencedScopes) {
+          this.thisReferencedScopes = [];
         }
-        this.referencedScopes.push(scope);
+        this.thisReferencedScopes.push(scope);
         return;
       }
       scope = scope.parent;
@@ -59,6 +59,12 @@ export default function ({types: t, template}: {types: BabelTypes, template: Bab
   const declarationTemplate = template(`
     var NAME = VALUE;
   `);
+  const symbolTemplate = template(`
+    new Symbol(NAME)
+  `);
+  const thisMemberReferenceTemplate = template(`
+    this[METHOD]
+  `);
   return {
     visitor: {
       Function (path: NodePath) {
@@ -72,14 +78,33 @@ export default function ({types: t, template}: {types: BabelTypes, template: Bab
         // or the global scope.
         const innerScope = new InnerScopeVisitor();
         path.traverse(innerScope);
+        const thisReferencedScopes = uniqueScopes(innerScope.thisReferencedScopes || []);
         const referencedScopes = uniqueScopes(innerScope.referencedScopes || []);
+        const allReferencedScopes = uniqueScopes([
+          ...(innerScope.referencedScopes || []),
+          ...thisReferencedScopes
+        ]);
         const targetScope = deepestScopeOf(
           path,
-          referencedScopes
+          allReferencedScopes
             .concat(path.scope.getProgramParent())
             .filter(scope => scope !== path.scope)
-         );
-        if (!targetScope || targetScope === path.scope.parent) {
+        );
+        if (!targetScope) return;
+        if (targetScope === path.scope.parent) {
+          if (
+            this.opts.methods &&
+            targetScope.path.isClassMethod() &&
+            thisReferencedScopes.indexOf(targetScope) !== -1 &&
+            referencedScopes.indexOf(targetScope) === -1
+          ) {
+            const parentScope: Scope = targetScope.parent;
+            const containingClassBodyPath: NodePath = targetScope.path.parentPath;
+            const id = parentScope.generateUidIdentifierBasedOnNode(path.node.id || path.node, 'hoistedMethod');
+            parentScope.push({kind: 'const', id, init: symbolTemplate({NAME: t.stringLiteral(id.name)}).expression});
+            containingClassBodyPath.unshiftContainer('body', t.classProperty(id, Object.assign({}, path.node, {shadow: true}), true));
+            path.replaceWith(thisMemberReferenceTemplate({METHOD: id}).expression);
+          }
           return;
         }
         if (path.node.id) {

diff --git a/test/fixtures/handle-references-to-this/actual.js b/test/fixtures/handle-references-to-this/actual.js
@@ -5,7 +5,7 @@
 
 class A {
     method() {
-      // FIXME: not hoisted but we could make it a "private" method
+      // NOTE: not hoisted
       return () => this;
     }
 }

diff --git a/test/fixtures/handle-references-to-this/expected.js b/test/fixtures/handle-references-to-this/expected.js
@@ -7,7 +7,7 @@ var _this = this;
 
 class A {
   method() {
-    // FIXME: not hoisted but we could make it a "private" method
+    // NOTE: not hoisted
     return () => this;
   }
 }

diff --git a/test/fixtures/hoist-nested-methods-if-options.methods-true/actual.js b/test/fixtures/hoist-nested-methods-if-options.methods-true/actual.js
@@ -0,0 +1,20 @@
+class A {
+  outer () {
+    // NOTE: hoisted
+    (function () {})();
+  }
+}
+
+class B {
+  outer () {
+    // NOTE: hoisted to bound method
+    (() => this)();
+  }
+}
+
+class C {
+  static outer () {
+    // NOTE: hoisted to static method
+    console.log((() => this)());
+  }
+}
diff --git a/test/fixtures/hoist-nested-methods-if-options.methods-true/expected.js b/test/fixtures/hoist-nested-methods-if-options.methods-true/expected.js
@@ -0,0 +1,29 @@
+const _hoistedMethod = new Symbol("_hoistedMethod"),
+      _hoistedMethod2 = new Symbol("_hoistedMethod2");
+
+var _hoistedAnonymousFunc2 = function () {};
+
+class A {
+  outer() {
+    // NOTE: hoisted
+    _hoistedAnonymousFunc2();
+  }
+}
+
+class B {
+  [_hoistedMethod] = () => this;
+
+  outer() {
+    // NOTE: hoisted to bound method
+    this[_hoistedMethod]();
+  }
+}
+
+class C {
+  [_hoistedMethod2] = () => this;
+
+  static outer() {
+    // NOTE: hoisted to static method
+    console.log(this[_hoistedMethod2]());
+  }
+}
diff --git a/test/fixtures/hoist-nested-methods-if-options.methods-true/options.json b/test/fixtures/hoist-nested-methods-if-options.methods-true/options.json
@@ -0,0 +1,3 @@
+{
+  "plugins": [["transform-hoist-nested-functions", {"methods": true}]]
+}
diff --git a/test/fixtures/not-hoist-mutated-funcs/actual.js b/test/fixtures/not-hoist-mutated-funcs/actual.js
@@ -35,3 +35,19 @@
   function inner(param) {}
   inner.name;
 })();
+
+(class {
+  outer() {
+    // FIXME: unsafely hoisted
+    const inner = () => {};
+    inner.someProp = 1;
+  }
+});
+
+(class {
+  outer() {
+    // NOTE: hoisted to bound method
+    const inner = () => this.constructor.name;
+    inner.name;
+  }
+});
diff --git a/test/fixtures/not-hoist-mutated-funcs/expected.js b/test/fixtures/not-hoist-mutated-funcs/expected.js
@@ -1,3 +1,5 @@
+const _hoistedMethod = new Symbol("_hoistedMethod");
+
 (function () {
   // NOTE: not hoisted
   function inner(param) {}
@@ -50,4 +52,24 @@ _inner4 = function inner(param) {};
   var inner = _inner4;
 
   inner.name;
-})();
+})();
+
+var _hoistedAnonymousFunc2 = () => {};
+
+(class {
+  outer() {
+    // FIXME: unsafely hoisted
+    const inner = _hoistedAnonymousFunc2;
+    inner.someProp = 1;
+  }
+});
+
+(class {
+  [_hoistedMethod] = () => this.constructor.name;
+
+  outer() {
+    // NOTE: hoisted to bound method
+    const inner = this[_hoistedMethod];
+    inner.name;
+  }
+});
diff --git a/test/fixtures/not-hoist-mutated-funcs/options.json b/test/fixtures/not-hoist-mutated-funcs/options.json
@@ -0,0 +1,3 @@
+{
+  "plugins": [["transform-hoist-nested-functions", {"methods": true}]]
+}
diff --git a/test/fixtures/not-hoist-nested-methods-by-default/actual.js b/test/fixtures/not-hoist-nested-methods-by-default/actual.js
@@ -0,0 +1,20 @@
+class A {
+  outer () {
+    // NOTE: hoisted
+    (function () {})();
+  }
+}
+
+class B {
+  outer () {
+    // NOTE: not hoisted (!options.methods)
+    (() => this)();
+  }
+}
+
+class C {
+  static outer () {
+    // NOTE: not hoisted (!options.methods)
+    console.log((() => this)());
+  }
+}
diff --git a/test/fixtures/not-hoist-nested-methods-by-default/expected.js b/test/fixtures/not-hoist-nested-methods-by-default/expected.js
@@ -0,0 +1,22 @@
+var _hoistedAnonymousFunc2 = function () {};
+
+class A {
+  outer() {
+    // NOTE: hoisted
+    _hoistedAnonymousFunc2();
+  }
+}
+
+class B {
+  outer() {
+    // NOTE: not hoisted (!options.methods)
+    (() => this)();
+  }
+}
+
+class C {
+  static outer() {
+    // NOTE: not hoisted (!options.methods)
+    console.log((() => this)());
+  }
+}
diff --git a/test/fixtures/not-hoist-nested-methods-by-default/options.json b/test/fixtures/not-hoist-nested-methods-by-default/options.json
@@ -0,0 +1,3 @@
+{
+  "plugins": ["transform-hoist-nested-functions"]
+}
diff --git a/test/fixtures/not-hoist-nested-methods-if-options.methods-false/actual.js b/test/fixtures/not-hoist-nested-methods-if-options.methods-false/actual.js
@@ -0,0 +1,20 @@
+class A {
+  outer () {
+    // NOTE: hoisted
+    (function () {})();
+  }
+}
+
+class B {
+  outer () {
+    // NOTE: not hoisted (!options.methods)
+    (() => this)();
+  }
+}
+
+class C {
+  static outer () {
+    // NOTE: not hoisted
+    console.log((() => this)());
+  }
+}
diff --git a/test/fixtures/not-hoist-nested-methods-if-options.methods-false/expected.js b/test/fixtures/not-hoist-nested-methods-if-options.methods-false/expected.js
@@ -0,0 +1,22 @@
+var _hoistedAnonymousFunc2 = function () {};
+
+class A {
+  outer() {
+    // NOTE: hoisted
+    _hoistedAnonymousFunc2();
+  }
+}
+
+class B {
+  outer() {
+    // NOTE: not hoisted (!options.methods)
+    (() => this)();
+  }
+}
+
+class C {
+  static outer() {
+    // NOTE: not hoisted
+    console.log((() => this)());
+  }
+}
diff --git a/test/fixtures/not-hoist-nested-methods-if-options.methods-false/options.json b/test/fixtures/not-hoist-nested-methods-if-options.methods-false/options.json
@@ -0,0 +1,3 @@
+{
+  "plugins": [["transform-hoist-nested-functions", {"methods": false}]]
+}