NullVoxPopuli / rollup-plugin-glimmer-template-tag

Rollup plugin for providing support for both gjs and gts file formats
MIT License
2 stars 1 forks source link

glimmerTemplateTag produces wrong scope at certain cases #14

Open candunaj opened 1 year ago

candunaj commented 1 year ago

When a component yields hash with components, as in the following example:

<!-- special.gjs -->
const components = { button: hbs`<button>...</button>`};
{{yield components}}
<!-- my-button.gjs -->
import Special from './special';
<template>
  <Special as |b|>
    <b.button />
  </Special>
</template>

then rollup plugin glimmerTemplateTag generates 'b' in the scope. 'b' is undefined in the generated javascript file as you can see below:

// my-button.js
import templateOnly from '@ember/component/template-only';
import { setComponentTemplate } from '@ember/component';
import { precompileTemplate } from '@ember/template-compilation';
import Special from './special.js';

var myButton = setComponentTemplate(precompileTemplate(`
  <Special as |b|>
    <b.button />
  </Special>
`, {
  strictMode: true,
  scope: () => ({
    Special,
    b
  })
}), templateOnly("my-button", "_myButton"));

export { myButton as default };

Expected result

'b' shouldn't be in the scope.

{
  ...,
  scope: () => ({
    Special,
  })
}

I have prepared a repo where the issue is simulated.

NullVoxPopuli commented 1 year ago

The timing on this is pretty good! your @glimmer/syntax in your lock file needs to be 0.84.3, I think

candunaj commented 1 year ago

I have now ensured that @glimmer/syntax is 0.84.3. But there is the same issue. I am not sure if I understand your answer correctly. Is it already fixed in a specific version of @glimmer/syntax?

NullVoxPopuli commented 1 year ago

Yeah,

NullVoxPopuli commented 1 year ago

j/k, the test does mention some earlier in the template. I'm gonna pull down your reproduction and see what's up

NullVoxPopuli commented 1 year ago

I ran: pnpm dedupe and it told me peers were missing:

gjs-bug
└─┬ rollup-plugin-glimmer-template-tag 0.3.0
  ├── ✕ missing peer ember-source@"^4.0.0 || ^5.0.0"
  └── ✕ missing peer ember-template-imports@^3.4.1
Peer dependencies that should be installed:
  ember-source@"^4.0.0 || ^5.0.0"  ember-template-imports@^3.4.1 

so.. installing ember-source and ember-template-imports in the addon folder.
ember-source reports that it requires:

Peer dependencies that should be installed:
  @glimmer/component@^1.1.2  webpack@">=5.0.0 <6.0.0"

which... is kind of a lie. Let's ignore those packages.
adding to the pnpm config in the root package.json

    "peerDependencyRules": {
      "ignoreMissing": [
        "@glimmer/component",
        "webpack"
      ]
    }

and now, the only peer issue is about the test-app, which we can ignore, because we're not looking at the test app.

At this point, the issue still exists, which is suspicious. Some dependency checking:

❯ pnpm ls @glimmer/syntax -r --depth 20
Legend: production dependency, optional only, dev only

gjs-bug@0.0.0 /home/nvp/Development/tmp/gjs-scope-bug/gjs-bug

devDependencies:
ember-template-lint 4.18.2
└─┬ ember-template-recast 6.1.3
  └── @glimmer/syntax 0.84.3

test-app@0.0.0 /home/nvp/Development/tmp/gjs-scope-bug/test-app

devDependencies:
ember-template-lint 5.7.1
└─┬ ember-template-recast 6.1.3
  └── @glimmer/syntax 0.84.3
eslint-plugin-ember 11.4.8
└── @glimmer/syntax 0.84.3

❯ pnpm ls ember-template-imports -r --depth 20
Legend: production dependency, optional only, dev only

gjs-bug@0.0.0 /home/nvp/Development/tmp/gjs-scope-bug/gjs-bug

devDependencies:
ember-template-imports 3.4.2
ember-template-lint 4.18.2
└── ember-template-imports 3.4.2
rollup-plugin-glimmer-template-tag 0.3.0
└── ember-template-imports 3.4.2 peer

test-app@0.0.0 /home/nvp/Development/tmp/gjs-scope-bug/test-app

devDependencies:
ember-template-lint 5.7.1
└── ember-template-imports 3.4.2
eslint-plugin-ember 11.4.8
└── ember-template-imports 3.4.2

:thinking:

NullVoxPopuli commented 1 year ago

I opened a PR here: https://github.com/glimmerjs/glimmer-vm/pull/1412

it'll be a while before that is reviewed and propagated throughout the ecosystem.