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

gts files are left with side-effecting imports #195

Open
NullVoxPopuli opened this issue Sep 7, 2023 · 11 comments
Open

gts files are left with side-effecting imports #195

NullVoxPopuli opened this issue Sep 7, 2023 · 11 comments
Labels
bug Something isn't working

Comments

@NullVoxPopuli
Copy link
Collaborator

NullVoxPopuli commented Sep 7, 2023

I currently get this messaging in my rollup build when updating to the latest blueprint:

[js] (!) Unused external imports
[js] TOC imported from external module "@ember/component/template-only" but never used in "src/components/dialog.gts", "src/components/-private/typed-elements.gts", "src/components/external-link.gts", "src/components/form.gts", "src/components/portal-targets.gts", "src/components/progress.gts", "src/components/shadowed.gts", "src/components/portal.gts", "src/components/popover.gts", "src/components/toggle.gts" and "src/components/switch.gts".
[js] ModifierLike,WithBoundArgs imported from external module "@glint/template" but never used in "src/components/dialog.gts", "src/components/progress.gts", "src/components/popover.gts" and "src/components/switch.gts".
[js] default imported from external module "@ember/routing/router-service" but never used in "src/components/link.gts".
[js] Middleware,MiddlewareData imported from external module "@floating-ui/dom" but never used in "src/components/popover.gts".
[js] Signature imported from external module "ember-velcro/modifiers/velcro" but never used in "src/components/popover.gts".
[js] (!) Generated an empty chunk
[js] "template-registry"
[js] created dist in 738ms

Repro here: universal-ember/ember-primitives#114

Example output:

       │ File: dist/components/switch.js
───────┼───────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   1   │ import { fn, hash } from '@ember/helper';
   2   │ import { on } from '@ember/modifier';
   3   │ import { cell } from 'ember-resources';
   4   │ import { uniqueId } from '../utils.js';
   5   │ import { Label } from './-private/typed-elements.js';
   6   │ import { toggleWithFallback } from './-private/utils.js';
   7   │ import { templateOnly } from '@ember/component/template-only';
   8   │ import '@glint/template';
   9   │ import { precompileTemplate } from '@ember/template-compilation';
  10   │ import { setComponentTemplate } from '@ember/component';
  11   │ 

The main issue being the remaining import @glint/template -- which is a type-only package, and in the real code, I have:

import type { WithBoundArgs } from '@glint/template';

So the whole thing should be removed in the emitted js

@NullVoxPopuli
Copy link
Collaborator Author

I have a reduced example here: https://stackblitz.com/edit/stackblitz-starters-rbahsf?description=Starter%20project%20for%20Node.js,%20a%20JavaScript%20runtime%20built%20on%20Chrome%27s%20V8%20JavaScript%20engine&file=babel.config.json,package.json,tsconfig.json,rollup.config.mjs,src%2Findex.ts,dist%2Findex.js&title=node.new%20Starter

Having a hard time reproducing the example -- it appears to have something to do with my code, as my simple examples don't reproduce the issue, but when I copy my addon's src folder into the default v2 addon blueprint output, the issue reproduces.

@NullVoxPopuli
Copy link
Collaborator Author

I have a repro using the v2 addon blueprint here:
https://stackblitz.com/edit/stackblitz-starters-qner5x?

output dist/index.js

import '@glint/template';
import Component from '@glimmer/component';

class Hello extends Component {}

export { Hello };
//# sourceMappingURL=index.js.map

@NullVoxPopuli
Copy link
Collaborator Author

It happens when you forget import type!!

So,

import type { Thing } from 'whatever';

will be fully removed

but

import { type Thing } from 'whatever';

will not be.

so we probably need a lint to prefer the import type

@bartocc
Copy link
Contributor

bartocc commented Sep 27, 2023

Please, re-open this issue as it is not fixed.

Repro repo here with stackblitz app
https://github.com/bartocc/stackblitz-starters-pyxvkz

When stackblitz has finished launching the app, check the file my-addon/dist/index.js, it'll contain the culprit import '@glint/template';

This only happens with the .gts extension.
Change my-addon/src/index.gts to my-addon/src/index.ts and the bug is gone

@NullVoxPopuli NullVoxPopuli reopened this Sep 27, 2023
@NullVoxPopuli NullVoxPopuli changed the title v2.5 does not remove the entirety of type imports, leaving side-effecting imports gts files are left with side-effecting imports Sep 27, 2023
@NullVoxPopuli NullVoxPopuli added the bug Something isn't working label Sep 27, 2023
@lukasnys
Copy link

I'm having the same issue here: https://github.com/lukasnys/ember-radix-ui.

No matter if I use import { WithBoundArgs } from '@glint/template';, import type { WithBoundArgs } from '@glint/template'; or import { type WithBoundArgs } from '@glint/template';. The import '@glint/template'; in the dist remains causing the consuming test-app to crash.

@NullVoxPopuli
Copy link
Collaborator Author

Made a stackblitz of the above repo if folks don't want to clone: https://stackblitz.com/edit/github-yl7xy1?file=packages%2Faccordion%2Fsrc%2Fcomponents%2Faccordion.gts&file=packages%2Faccordion%2Fdist%2Fcomponents%2Faccordion.js
image

@NullVoxPopuli
Copy link
Collaborator Author

resolved by ensuring that content-tag in your lockfile is up to date (at least 1.1.2).
This'll be fixed shortly with a new minimum @embroider/addon-dev version.

@bartocc
Copy link
Contributor

bartocc commented Oct 9, 2023

I confirm that using content-tag 1.1.2 fixes the issue 👍
thx @NullVoxPopuli and @ef4 for the fix 👍

@lukasnys
Copy link

lukasnys commented Oct 9, 2023

Same for me, I tried updating to [email protected] in the Stackblitz example @NullVoxPopuli provided and then it's fixed 👍. Thanks!

@enspandi
Copy link

enspandi commented Oct 9, 2023

Same here: updating content-tag removed the import '@glint/template' line, thanks 👏!


But I just found that we have a similar issue with ember-concurrency, where a import 'ember-concurrency'; remains in the compiled output... is it maybe related or should it be addressed in EC?

Some details

Build output

[js] 
[js] > [email protected] build:js
[js] > rollup --config
[js] 
[types] 
[types] > [email protected] build:types
[types] > glint --declaration
[types] 
[js] 
[js]  → dist...
[js] (!) Unresolved dependencies
[js] https://rollupjs.org/troubleshooting/#warning-treating-module-as-external-dependency
[js] ember-concurrency/-private/async-arrow-runtime (imported by "src/components/hello.gts")
[js] ember-concurrency (imported by "src/components/hello.gts")
[js] (!) Generated empty chunks
[js] "index" and "template-registry"
[js] (!) Unused external imports
[js] task imported from external module "ember-concurrency" but never used in "src/components/hello.gts".
[js] created dist in 1.1s
[js] npm run build:js exited with code 0
[types] npm run build:types exited with code 0

Component / GTS

import { task } from 'ember-concurrency';

export default class Hello extends Component<Signature> {
  // ...
  testTask = task({ drop: true }, async () => {
    console.log('hello');
  });
}

Compiled / JS

import { buildTask } from 'ember-concurrency/-private/async-arrow-runtime';
import 'ember-concurrency';

....

class Hello extends Component {
  constructor(...args) {
    super(...args);
    _defineProperty(this, "testTask", buildTask(() => ({
      context: this,
      generator: function* () {
        console.log('hello');
      }
    }), {
      drop: true
    }, "testTask", null));
  }

@NullVoxPopuli
Copy link
Collaborator Author

yeah, sounds like a bug in their babel plugin -- they have enough knowledge of what they need to do to remove that themselves (babel can't do it, because it's not safe), but ember-concurrency knows that there is no reason to use a side-effecting import from itself.

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

No branches or pull requests

4 participants