pass correct scope to glimmer #33
Conversation
src/plugin.ts
Outdated
| @@ -483,7 +509,8 @@ function insertCompiledTemplate<EnvSpecificOptions>( | |||
| backingClass: NodePath<Parameters<typeof t.callExpression>[1][number]> | undefined | |||
| ) { | |||
| let t = babel.types; | |||
| let scopeLocals = buildScopeLocals(userTypedOptions, config, template); | |||
| target.state = state; | |||
There was a problem hiding this comment.
why's this needed? why not pass state to buildScopeLocals?
|
Not ready after all, block params scope should not apply to own attributes or modifiers. Currently that happens. Not sure why the tests are passing. |
src/plugin.ts
Outdated
| enter(_node, path) { | ||
| const blockParams = (path.parentNode as any)?.blockParams; | ||
| if (blockParams && ['children', 'body'].includes(path.parentKey!)) { | ||
| astScope.push(blockParams); |
There was a problem hiding this comment.
we just push the block params array, since its the same instance we can easily remove it at exit
src/plugin.ts
Outdated
| @@ -437,6 +490,9 @@ function buildPrecompileOptions<EnvSpecificOptions>( | |||
| }, | |||
| }; | |||
|
|
|||
| const locals: string[] = []; | |||
| output.plugins?.ast?.push(discoverLocals(target, state, locals, scope)); | |||
There was a problem hiding this comment.
we push our own locals discovery ast plugin, which mutates the locals passed to precompile.
@chancancode if you have a better option, now is the chance :)
src/plugin.ts
Outdated
| @@ -513,6 +569,8 @@ function insertCompiledTemplate<EnvSpecificOptions>( | |||
| configFile: false, | |||
| }) as t.File; | |||
|
|
|||
| scopeLocals.locals.length = 0; | |||
There was a problem hiding this comment.
reset scope locals and set it to the ones discovered at the end. As ast plugins could have removed previously added locals.
| @@ -574,14 +632,14 @@ function insertTransformedTemplate<EnvSpecificOptions>( | |||
| let transformed = print(ast, { entityEncoding: 'raw' }); | |||
| if (target.isCallExpression()) { | |||
| (target.get('arguments.0') as NodePath<t.Node>).replaceWith(t.stringLiteral(transformed)); | |||
| if (!scopeLocals.isEmpty()) { | |||
There was a problem hiding this comment.
scope locals could have changed from something, to empty, we need to remove existing scope
| @@ -31,7 +31,15 @@ export class ScopeLocals { | |||
| } | |||
|
|
|||
| entries() { | |||
| return Object.entries(this.#mapping); | |||
There was a problem hiding this comment.
only use mappings for actual scope
47c1e57 to
069fa3e
Compare
Made while pairing with @patricklx. - keep a single locals list instead of synchronizing two different ones - update jsutils to always rely directly on Babel's bindings, not ScopeLocals - reuse the hbs-scope-traversal scope from jsutils instead of introducing a second implementation
|
@patricklx and I paired on this and it's getting close to done. The remaining task is to add a test for removing things from locals, because we think we might not have that case done yet. |
The explicit scope form doesn't automatically bind to javascript. It's different from the implicit-scope-form, which does. The only modifications we should be making to the scope bag in explicit form are modifications caused by AST transforms. (Dropping unused is also a safe transformation and can stay.)
|
While testing this I realized we're now doing too much. In the explicit scope form, we're not supposed to magically bind to javascript values in scope. That's only supposed to happen to the implicit form (the one that has The only modifications we should be making to the scope bag in explicit form are modifications explicitly asked for by AST transforms. (Dropping unused names is also a safe transformation and can stay.) |
|
@ef4 reverted some parts and updated tests |
It doesn't need to read from ScopeLocals.
It doesn't seem intentional that it got an extra feature added into it.
|
Thanks for all the work on this. Released in 2.2.2. |
It was in a sense useful that it would prematurely catch missing imports or definitions for template locals.
But with today's state of glint and eslint we do not really need this anymore.
It also prevents use of web components and specifying components for known html elements did also not work.
also fixes usage of svg elments
fixes glimmerjs/glimmer-vm#1550
look at
df34cb4 for tests diff