Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add failing test for v4 / content-tag not respecting imports, demonstrate fix by rolling for a new lockfile and cleaning up resulting peer errors #211

Merged
merged 6 commits into from
Nov 2, 2023

Conversation

NullVoxPopuli
Copy link
Collaborator

@NullVoxPopuli NullVoxPopuli commented Oct 22, 2023

Initial failing test here: #206
Issue: #207
Blocked by: emberjs/babel-plugin-ember-template-compilation#30

The problem is that we're doing the "lazy way" of compiling, where we don't produce the scope bag:
prior to this PR, the added test is converted

import { template } from "@ember/template-compiler";
import { module, test } from 'qunit';
import { setupRenderingTest } from 'ember-qunit';
import { render } from '@ember/test-helpers';
import { precompileTemplate } from '@ember/template-compilation';
import Component from '@glimmer/component';
import { on } from '@ember/modifier';
import GjsTest from 'dummy/components/gjs-test';

// ...

    test('it works with imports', async function(assert) {
        let didIt = ()=>assert.step('did it');
        await render(template(`
        <button {{on 'click' didIt}}>
          step
        </button>
      `, {
            eval () {
                return eval(arguments[0]);
            }
        }));
        await click('button');
        assert.verifySteps([
            'did it'
        ]);
    });

the imports are correct -- on is there.
However, eval(arguments[0]) doesn't use the needed scope:

ReferenceError: on is not defined
                at Object.scope (http://localhost:7357/assets/tests.js:50:25)

When compiling to something runnable, the resulting code is:

    (0, _qunit.test)('it works with imports', async function (assert) {
      let didIt = () => assert.step('did it');
      await (0, _testHelpers.render)((0, _component2.setComponentTemplate)((0, _templateFactory.createTemplateFactory)(
      /*
        
              <button {{on 'click' didIt}}>
                step
              </button>
            
      */
      {
        "id": "GLgVGNG5",
        "block": "[[[1,\"\\n        \"],[11,\"button\"],[4,[32,0],[\"click\",[32,1]],null],[12],[1,\"\\n          step\\n        \"],[13],[1,\"\\n      \"]],[],false,[]]",
        "moduleName": "/home/nullvoxpopuli/Development/OpenSource/ember-template-imports/dummy/tests/integration/gjs-test.js",
        "scope": () => [on, didIt],
        "isStrictMode": true
      }), (0, _templateOnly.default)()));
      await click('button');
      assert.verifySteps(['did it']);
    });

now this looks like it should work, but on doesn't exist in our AMD format.
the define module has:

define(
  "dummy/tests/integration/gjs-test", 
  [
    "qunit", "ember-qunit", "@ember/test-helpers", 
    "@glimmer/component", "dummy/components/gjs-test", 
    "@ember/template-factory", "@ember/component", 
    "@ember/component/template-only"
  ], 
  function (
    _qunit, _emberQunit, _testHelpers, 
    _component, _gjsTest, 
   _templateFactory, _component2, 
   _templateOnly
) {

which omits @ember/modifier -- likely due to the default babel transform knowing that eval(arguments[0]) needs template parsing to figure out what imports need.

@ef4
Copy link
Collaborator

ef4 commented Oct 23, 2023

Thanks for the reproduction branch.

This description of the bug is incorrect:

The problem is that we're doing the "lazy way" of compiling, where we don't produce the scope bag:

We only physically emit the scope bag form when we're doing addon compilation. In an app we skip directly from the eval-form to wire format. That is the whole reason eval-form exists: it's cheap and it avoids the cost of ever separately computing the scope bag when we're trying to go straight to wire format anyway.

This bug is a duplicate of emberjs/babel-plugin-ember-template-compilation#30

The dummy app in ember-template-imports uses enableTypescriptTransform: true, which applies @babel/plugin-transform-typescript, which is having the effect described in the linked issue.

@ef4
Copy link
Collaborator

ef4 commented Oct 23, 2023

(FWIW, I don't know why the dummy app uses enableTypescriptTransform, because it doesn't seem to include any typescript, and if you turn that flag off you can make your reproduction test pass. But this is a real bug anyway for apps that actually do need typescript.)

@ef4
Copy link
Collaborator

ef4 commented Nov 1, 2023

This should be fixed in babel-plugin-ember-template-compilation 2.2.1.

@NullVoxPopuli NullVoxPopuli marked this pull request as ready for review November 1, 2023 23:08
@NullVoxPopuli NullVoxPopuli changed the title Add failing test for v4 / content-tag not respecting imports Add failing test for v4 / content-tag not respecting imports, demonstrate fix by rolling for a new lockfile and cleaning up resulting peer errors Nov 1, 2023
@@ -67,7 +67,7 @@
"qunit": "^2.14.0",
"qunit-dom": "^3.0.0",
"release-it": "^16.2.1",
"typescript": "^4.5.2",
"typescript": "^4.9.5",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

required by release-it for some reason

"@release-it-plugins/lerna-changelog": "^6.0.0",
"@typescript-eslint/eslint-plugin": "^6.8.0",
"@typescript-eslint/parser": "^6.8.0",
"ember-auto-import": "^2.6.3",
"ember-cli": "~4.12.1",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this version isn't compat with ember-qunit v8

"@glimmer/syntax": "0.84.3",
"@glimmer/tracking": "^1.0.3",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sufficiently modern ember-source requires these changes

@ef4 ef4 added the internal label Nov 2, 2023
@ef4 ef4 merged commit 938509e into master Nov 2, 2023
16 checks passed
@ef4 ef4 deleted the test-imports branch November 2, 2023 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants