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

Remove extraneous import from @ember/template-compiler #51

Merged
merged 5 commits into from
May 8, 2024

Conversation

NullVoxPopuli
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli commented May 7, 2024

First commit:

demonstrates a failing test where @ember/template-compiler is left in the output code.
This manifests itself in the default blueprint when two components exist in the same file:

reproduction is simple

Input code:

import Component from '@glimmer/component';

export default class Test extends Component {
  foo = 1;

  <template><Icon /></template>
}

const Icon = <template>Icon</template>;

Which is intermediately (still correct):

import { template } from "@ember/template-compiler";
import Component from '@glimmer/component';
export default class Test extends Component {
    foo = 1;
    static{
        template("<Icon />", {
            component: this,
            eval () {
                return eval(arguments[0]);
            }
        });
    }
}
const Icon = template("Icon", {
    eval () {
        return eval(arguments[0]);
    }
});

Output code:

import '@ember/template-compiler';
import Component from '@glimmer/component';
import { precompileTemplate } from '@ember/template-compilation';
import { setComponentTemplate } from '@ember/component';
import templateOnly from '@ember/component/template-only';

class Test extends Component {
  foo = 1;
  static {
    setComponentTemplate(precompileTemplate("<Icon />", {
      strictMode: true,
      scope: () => ({
        Icon
      })
    }), this);
  }
}
const Icon = setComponentTemplate(precompileTemplate("Icon", {
  strictMode: true
}), templateOnly());

export { Test as default };
//# sourceMappingURL=test.js.map

Rollup detects that { template } is unused, but then leaves a side-effecting import for @ember/template-compiler

@NullVoxPopuli NullVoxPopuli marked this pull request as ready for review May 7, 2024 22:02
src/plugin.ts Outdated
// We need to refresh ourselves
// This is expensive, but required so that maybePruneImport
// has a chance to prune imports when their specifiers are no longer used.
state.program.scope.crawl();
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, so the previous release just cleaned up all our need for scope.crawl. It appears that this one place was not covered.

The fix for this will go into babel-import-util. Using util.removeImport is supposed to correct the scope references.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking closer I didn't explain correctly in my comment above, but the basic point is still true: we don't need scope.crawl when we're the ones directly responsible for removing the reference. We should be updating the scope at that point so it stays correct.

@ef4 ef4 added the bug Something isn't working label May 8, 2024
@ef4 ef4 merged commit e043d54 into main May 8, 2024
7 checks passed
@delete-merged-branch delete-merged-branch bot deleted the remove-extra-import-from@ember-template-compiler branch May 8, 2024 02:04
@ef4
Copy link
Contributor

ef4 commented May 8, 2024

Thanks, the test was helpful in narrowing this down.

@github-actions github-actions bot mentioned this pull request May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants