embroider-build / embroider

Compiling Ember apps into spec-compliant, modern Javascript.
MIT License
336 stars 136 forks source link

v1 addon with gts + ember-template-imports failing under Embroider #2119

Open simonihmig opened 1 week ago

simonihmig commented 1 week ago

When a v1 addon has gts components and ember-template-imports, this is working under Classic, but failing under Embroider (stable). Imports of components used in <template> get stripped away (in the rewritten package), thereby leading to Attempted to invoke a component that was not in scope in a strict mode template errors.

Found some prior discussion around that on Discord between @Windvis and @NullVoxPopuli, where it was suggested to make the v1 babel typescript config same as for v2 addons. Which @Windvis did here, and I can confirm in my own case that adding onlyRemoveTypeImports fixes the issue.

However, we should get this to work out of the box, right? It was suggested to add this config to ember-cli-babel by default, but not sure if that is viable? Couldn't this break stuff when imports are only used for types, but not declared explicitly with e.g. import type?

Also puzzling to me is the fact that this only causes errors under Embroider? (which is the reason I opened this issue here, even when the fix might go somewhere else)

patricklx commented 1 week ago

is the babel plugin babel-plugin-ember-template-compilation running there? Can you try removing the typescript babel plugin config and add babel-plugin-ember-template-compilation instead?

I think there was a condition to only include it if there are any hbs files. I might be wrong

patricklx commented 1 week ago

found it: https://github.com/embroider-build/embroider/blob/fae40599d0799ed72f586c625f9ba0a02a3a4b4a/packages/compat/src/v1-addon.ts#L199 It says it's only enabled for inline hbs, i think gjs/gts is kind of it

patricklx commented 1 week ago

this is what I did to get it to work: there was an issue that something is still removing the plugin if its in the list... so i had to make a re-export file

plugins: [
        require.resolve('ember-auto-import/babel-plugin'),
        [
          require.resolve('./babel-plugin-ember-template-compilation.js'), {
          compilerPath: require.resolve('ember-source/dist/ember-template-compiler.js'),
          targetFormat: 'hbs',
          enableLegacyModules: [
            'ember-cli-htmlbars',
            'ember-cli-htmlbars-inline-precompile',
            'htmlbars-inline-precompile',
            '@ember/template-compilation'
          ],
          transforms: [],
        }]
basz commented 6 days ago

hi @patricklx

Is that added to the babel section of ember-cli-build.js? And if so, I'm getting an "Cannot find module './babel-plugin-ember-template-compilation.js'" error.

patricklx commented 6 days ago

yes, you need to create the file ./babel-plugin-ember-template-compilation.js with the content

module.exports = require('babel-plugin-ember-template-compilation')
basz commented 6 days ago

yes that compiles. thank you. it does not resolve my initial issue though...

patricklx commented 6 days ago

so, this is to fix the issue in v1 addons, so the change needs to be done in the addon main entry file. https://github.com/appuniversum/ember-appuniversum/blob/master/index.js#L13 instead of the '@babel/plugin-transform-typescript' entry, use the one from https://github.com/embroider-build/embroider/issues/2119#issuecomment-2363883951

ef4 commented 6 days ago

onlyRemoveTypeImports is NOT supposed to be necessary on current versions of babel-plugin-ember-template-compilation. That is why we do everything in pre. https://github.com/emberjs/babel-plugin-ember-template-compilation/blob/cdbabfdbe12be6354b1bbb827a700952714e6d11/src/plugin.ts#L320. If there are cases that still need onlyRemoveTypeImports please make a reproduction.

patricklx commented 6 days ago

the problem is that babel-plugin-ember-template-compilation wasn't included. it probably will be with the next release of ember-template-imports

ef4 commented 6 days ago

it probably will be with the next release of ember-template-imports

That's not going to have any effect. Embroider doesn't let v1 addons decide whether or not to do their own template compilation.

But yeah, I'm starting to understand. I'll followup on the PR.

ef4 commented 6 days ago

I wrote up more explanation in https://github.com/embroider-build/embroider/pull/2120#issuecomment-2368236511 and this is a bug we can fix.

But also: it has long been recommended that v1 addon authors publish JS and not TS. Even ember-cli-typescript gave a ts:precompile command for that. Leaving the complexity for the app build is bad.

It's supported and we can keep fixing bugs, but please spread the word. People should not be publishing GTS (or GJS) to NPM. Compile it to javascript once instead of making every app author do it hundreds of times a day.

As for how to do that in a v1 addon: if I needed to do that I would probably start with the v2 blueprint and modify it slightly to publish in v1. The smallest thing that would probably work is:

  1. Run the build.
  2. Rename dist/_app_ to app
  3. Rename dist to addon.
  4. Use the standard v1 index.js file instead of the addon-shim one.
  5. Delete version: 2 from package.json.

Now you have a valid v1 addon that's all pre-built with rollup and doesn't need a dependency on ember-template-imports or typescript.

basz commented 6 days ago

in my case it are in repo (and private) addons. so i guess the js over ts argument does not apply here...

mansona commented 5 days ago

the js over ts argument does still apply because we're talking about module boundaries regardless if you're actually "publishing to npm". I have encouraged many people to imagine they are publishing to npm anyway and it has very often solved many problems for them 🎉

basz commented 5 days ago

sure, yet I really want to write TS... :-)

ef4 commented 5 days ago

He’s not saying to stop using TS. He’s saying to build each library’s TS to JS separately, so that all other packages only need to see the JS (and .d.ts).