embroider-build / addon-blueprint

Blueprint for v2-formatted Ember addons
MIT License
29 stars 25 forks source link

gts files are left with side-effecting imports #195

Open NullVoxPopuli opened 1 year ago

NullVoxPopuli commented 1 year ago

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: https://github.com/universal-ember/ember-primitives/pull/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 commented 1 year ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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

lukasnys commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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

lukasnys commented 11 months ago

Same for me, I tried updating to content-tag@1.1.2 in the Stackblitz example @NullVoxPopuli provided and then it's fixed 👍. Thanks!

enspandi commented 11 months ago

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


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 - Using latest EC @ 3.1.1 - Added EC babel transform to babel.config.json `"ember-concurrency/lib/babel-plugin-transform-ember-concurrency-async-tasks"` (https://github.com/machty/ember-concurrency/blob/master/lib/babel-plugin-transform-ember-concurrency-async-tasks.js) **Build output** ```bash [js] [js] > my-addon@0.0.0 build:js [js] > rollup --config [js] [types] [types] > my-addon@0.0.0 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** ```ts import { task } from 'ember-concurrency'; export default class Hello extends Component { // ... testTask = task({ drop: true }, async () => { console.log('hello'); }); } ``` **Compiled / JS** ```ts 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 commented 11 months ago

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.