Merge pull request #33 from patricklx/patch-3

pass correct scope to glimmer

Author: ef4

__tests__/tests.ts

@@ -996,6 +996,31 @@ describe('htmlbars-inline-precompile', function () {
     expect(transformed).toContain(`window.define('my-app/components/thing', thing)`);
   });
 
+  it('prevents inconsistent external manipulation of the locals array', function () {
+    let compatTransform: ExtendedPluginBuilder = (env) => {
+      return {
+        name: 'compat-transform',
+        visitor: {
+          Template() {
+            (env as any).locals.push('NewThing');
+          },
+        },
+      };
+    };
+
+    plugins = [[HTMLBarsInlinePrecompile, { targetFormat: 'hbs', transforms: [compatTransform] }]];
+
+    expect(() => {
+      transform(stripIndent`
+      import { precompileTemplate } from '@ember/template-compilation';
+      let NewThing = Thing;
+      export default function() {
+        const template = precompileTemplate('<Thing />');
+      }
+    `);
+    }).toThrow(/The only supported way to manipulate locals is via the jsutils API/);
+  });
+
   it('can emit side-effectful import', function () {
     let compatTransform: ExtendedPluginBuilder = (env) => {
       return {
@@ -1529,7 +1554,7 @@ describe('htmlbars-inline-precompile', function () {
 
   describe('scope', function () {
     it('correctly handles scope function (non-block arrow function)', function () {
-      let source = 'hello';
+      let source = '<foo /><bar/>';
       let spy = sinon.spy(compiler, 'precompile');
 
       transform(
@@ -1539,7 +1564,7 @@ describe('htmlbars-inline-precompile', function () {
     });
 
     it('correctly handles scope function (block arrow function)', function () {
-      let source = 'hello';
+      let source = '<foo /><bar/>';
       let spy = sinon.spy(compiler, 'precompile');
 
       transform(
@@ -1550,7 +1575,7 @@ describe('htmlbars-inline-precompile', function () {
     });
 
     it('correctly handles scope function (normal function)', function () {
-      let source = 'hello';
+      let source = '<foo /><bar/>';
       let spy = sinon.spy(compiler, 'precompile');
 
       transform(
@@ -1561,7 +1586,7 @@ describe('htmlbars-inline-precompile', function () {
     });
 
     it('correctly handles scope function (object method)', function () {
-      let source = 'hello';
+      let source = '<foo /><bar/>';
       let spy = sinon.spy(compiler, 'precompile');
 
       transform(
@@ -1571,7 +1596,7 @@ describe('htmlbars-inline-precompile', function () {
     });
 
     it('correctly handles scope function with coverage', function () {
-      let source = 'hello';
+      let source = '<foo /><bar/>';
       let spy = sinon.spy(compiler, 'precompile');
 
       transform(
@@ -1626,6 +1651,26 @@ describe('htmlbars-inline-precompile', function () {
         /Scope objects for `precompileTemplate` may only contain direct references to in-scope values, e.g. { bar } or { bar: bar }/
       );
     });
+
+    it('correctly removes not used scope', function () {
+      let spy = sinon.spy(compiler, 'precompile');
+      transform(`
+        import { precompileTemplate } from '@ember/template-compilation';
+        let foo, bar;
+        var compiled = precompileTemplate('<foo /><bar/>', { scope: () => ({ foo, bar, baz }) });
+      `);
+      expect(spy.firstCall.lastArg).toHaveProperty('locals', ['foo', 'bar']);
+    });
+
+    it('does not automagically add to scope when not using implicit-scope-form', function () {
+      let spy = sinon.spy(compiler, 'precompile');
+      transform(`
+        import { precompileTemplate } from '@ember/template-compilation';
+        let foo, bar;
+        var compiled = precompileTemplate('<foo /><bar/>', { scope: () => ({ bar }) });
+      `);
+      expect(spy.firstCall.lastArg).toHaveProperty('locals', ['bar']);
+    });
   });
 
   describe('implicit-scope-form', function () {
@@ -1660,9 +1705,7 @@ describe('htmlbars-inline-precompile', function () {
     // that's what the lint rules are for. When it comes to correctness, we need
     // our scope to behave like real Javascript, and Javascript doesn't care
     // whether you've (for example) capitalized your variable identifier.
-    //
-    // needs https://github.com/glimmerjs/glimmer-vm/pull/1421
-    it.skip('shadows html elements with locals', function () {
+    it('shadows html elements with locals', function () {
       plugins = [
         [
           HTMLBarsInlinePrecompile,
@@ -1681,9 +1724,9 @@ describe('htmlbars-inline-precompile', function () {
       );
 
       expect(transformed).toEqualCode(`
-        import templateOnly from "@ember/component/template-only";
-        import { setComponentTemplate } from "@ember/component";
         import { precompileTemplate } from "@ember/template-compilation";
+        import { setComponentTemplate } from "@ember/component";
+        import templateOnly from "@ember/component/template-only";
         let div = 1;
         export default setComponentTemplate(precompileTemplate('<div></div>', { scope: () => ({ div }), strictMode: true }), templateOnly());
       `);
@@ -1716,8 +1759,7 @@ describe('htmlbars-inline-precompile', function () {
       `);
     });
 
-    // needs https://github.com/glimmerjs/glimmer-vm/pull/1421
-    it.skip('leaves ember keywords alone when no local is defined', function () {
+    it('leaves ember keywords alone when no local is defined', function () {
       plugins = [
         [
           HTMLBarsInlinePrecompile,
@@ -1735,9 +1777,9 @@ describe('htmlbars-inline-precompile', function () {
       );
 
       expect(transformed).toEqualCode(`
-        import templateOnly from "@ember/component/template-only";
-        import { setComponentTemplate } from "@ember/component";
         import { precompileTemplate } from "@ember/template-compilation";
+        import { setComponentTemplate } from "@ember/component";
+        import templateOnly from "@ember/component/template-only";
         export default setComponentTemplate(precompileTemplate('{{hasBlock "thing"}}', { strictMode: true }), templateOnly());
       `);
     });

src/expression-parser.ts

@@ -107,7 +107,7 @@ export class ExpressionParser {
 
       res.add(propName, value.name);
       return res;
-    }, new ScopeLocals());
+    }, new ScopeLocals({ mode: 'explicit' }));
   }
 
   parseEval(

src/hbs-utils.ts

@@ -0,0 +1,28 @@
+import type { ASTv1, WalkerPath } from '@glimmer/syntax';
+
+export function astNodeHasBinding(target: WalkerPath<ASTv1.Node>, name: string): boolean {
+  let cursor: WalkerPath<ASTv1.Node> | null = target;
+  while (cursor) {
+    let parentNode = cursor.parent?.node;
+    if (
+      parentNode?.type === 'ElementNode' &&
+      parentNode.blockParams.includes(name) &&
+      // an ElementNode's block params are valid only within its children
+      parentNode.children.includes(cursor.node as ASTv1.Statement)
+    ) {
+      return true;
+    }
+
+    if (
+      parentNode?.type === 'Block' &&
+      parentNode.blockParams.includes(name) &&
+      // a Block's blockParams are valid only within its body
+      parentNode.body.includes(cursor.node as ASTv1.Statement)
+    ) {
+      return true;
+    }
+
+    cursor = cursor.parent;
+  }
+  return false;
+}

src/js-utils.ts

@@ -3,7 +3,7 @@ import type * as Babel from '@babel/core';
 import type { NodePath } from '@babel/traverse';
 import type { ASTPluginBuilder, ASTPluginEnvironment, ASTv1, WalkerPath } from '@glimmer/syntax';
 import type { ImportUtil } from 'babel-import-util';
-import { ScopeLocals } from './scope-locals';
+import { astNodeHasBinding } from './hbs-utils';
 
 interface State {
   program: NodePath<Babel.types.Program>;
@@ -16,23 +16,20 @@ export class JSUtils {
   #babel: typeof Babel;
   #state: State;
   #template: NodePath<t.Expression>;
-  #scopeLocals: ScopeLocals;
+  #addedBinding: (name: string) => void;
   #importer: ImportUtil;
 
   constructor(
     babel: typeof Babel,
     state: State,
     template: NodePath<t.Expression>,
-    // mapping of handlebars identifiers to javascript identifiers, as appears
-    // in the `scope` argument to precompileTemplate. This is both read and
-    // write -- we might put more stuff into it.
-    scopeLocals: ScopeLocals,
+    addedBinding: (name: string) => void,
     importer: ImportUtil
   ) {
     this.#babel = babel;
     this.#state = state;
     this.#template = template;
-    this.#scopeLocals = scopeLocals;
+    this.#addedBinding = addedBinding;
     this.#importer = importer;
 
     if (!this.#state.lastInsertedPath) {
@@ -71,9 +68,7 @@ export class JSUtils {
     let name = unusedNameLike(
       opts?.nameHint ?? 'a',
       (candidate) =>
-        this.#template.scope.hasBinding(candidate) ||
-        this.#scopeLocals.has(candidate) ||
-        astNodeHasBinding(target, candidate)
+        this.#template.scope.hasBinding(candidate) || astNodeHasBinding(target, candidate)
     );
     let t = this.#babel.types;
     let declaration: NodePath<t.VariableDeclaration> = this.#emitStatement(
@@ -84,8 +79,8 @@ export class JSUtils {
         ),
       ])
     );
-    declaration.scope.registerBinding('module', declaration.get('declarations.0') as NodePath);
-    this.#scopeLocals.add(name);
+    declaration.scope.registerBinding('let', declaration.get('declarations.0') as NodePath);
+    this.#addedBinding(name);
     return name;
   }
 
@@ -126,36 +121,34 @@ export class JSUtils {
       opts?.nameHint
     );
 
-    // If we're already referencing the imported name from the outer scope and
-    // it's not shadowed at our target location in the template, we can reuse
-    // the existing import.
-    if (
-      this.#scopeLocals.has(importedIdentifier.name) &&
-      !astNodeHasBinding(target, importedIdentifier.name)
-    ) {
+    // Simple base case: the JS name that's available is also unused at our spot
+    // in HBS, so just use it.
+    if (!astNodeHasBinding(target, importedIdentifier.name)) {
+      this.#addedBinding(importedIdentifier.name);
       return importedIdentifier.name;
     }
 
+    // The importedIdentifier that we have in Javascript is not usable within
+    // our HBS because it's shadowed by a block param. So we will introduce a
+    // second name via a variable declaration.
+    //
+    // The reason we don't force the import itself to have this name is that
+    // we might be re-using an existing import, and we don't want to go
+    // rewriting all of its callsites that are unrelated to us.
     let identifier = unusedNameLike(
       importedIdentifier.name,
-      (candidate) => this.#scopeLocals.has(candidate) || astNodeHasBinding(target, candidate)
+      (candidate) =>
+        this.#template.scope.hasBinding(candidate) || astNodeHasBinding(target, candidate)
     );
-    if (identifier !== importedIdentifier.name) {
-      // The importedIdentifier that we have in Javascript is not usable within
-      // our HBS because it's shadowed by a block param. So we will introduce a
-      // second name via a variable declaration.
-      //
-      // The reason we don't force the import itself to have this name is that
-      // we might be re-using an existing import, and we don't want to go
-      // rewriting all of its callsites that are unrelated to us.
-      let t = this.#babel.types;
-      this.#emitStatement(
-        t.variableDeclaration('let', [
-          t.variableDeclarator(t.identifier(identifier), importedIdentifier),
-        ])
-      );
-    }
-    this.#scopeLocals.add(identifier);
+    let t = this.#babel.types;
+    let declaration = this.#emitStatement(
+      t.variableDeclaration('let', [
+        t.variableDeclarator(t.identifier(identifier), importedIdentifier),
+      ])
+    );
+    declaration.scope.registerBinding('let', declaration.get('declarations.0') as NodePath);
+
+    this.#addedBinding(identifier);
     return identifier;
   }
 
@@ -223,33 +216,6 @@ function unusedNameLike(desiredName: string, isUsed: (name: string) => boolean):
   return candidate;
 }
 
-function astNodeHasBinding(target: WalkerPath<ASTv1.Node>, name: string): boolean {
-  let cursor: WalkerPath<ASTv1.Node> | null = target;
-  while (cursor) {
-    let parentNode = cursor.parent?.node;
-    if (
-      parentNode?.type === 'ElementNode' &&
-      parentNode.blockParams.includes(name) &&
-      // an ElementNode's block params are valid only within its children
-      parentNode.children.includes(cursor.node as ASTv1.Statement)
-    ) {
-      return true;
-    }
-
-    if (
-      parentNode?.type === 'Block' &&
-      parentNode.blockParams.includes(name) &&
-      // a Block's blockParams are valid only within its body
-      parentNode.body.includes(cursor.node as ASTv1.Statement)
-    ) {
-      return true;
-    }
-
-    cursor = cursor.parent;
-  }
-  return false;
-}
-
 /**
  * This extends Glimmer's ASTPluginEnvironment type to put our jsutils into meta
  */