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

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

Closed
simonihmig opened this issue Sep 20, 2024 · 18 comments · Fixed by #2120
Closed

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

simonihmig opened this issue Sep 20, 2024 · 18 comments · Fixed by #2120

Comments

@simonihmig
Copy link
Collaborator

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
Copy link
Contributor

patricklx commented Sep 20, 2024

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
Copy link
Contributor

found it:

private needsInlineHBS(): boolean {

It says it's only enabled for inline hbs, i think gjs/gts is kind of it

@patricklx
Copy link
Contributor

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
Copy link

basz commented Sep 23, 2024

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
Copy link
Contributor

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
Copy link

basz commented Sep 23, 2024

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

@patricklx
Copy link
Contributor

patricklx commented Sep 23, 2024

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 #2119 (comment)

@ef4
Copy link
Contributor

ef4 commented Sep 23, 2024

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
Copy link
Contributor

patricklx commented Sep 23, 2024

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
Copy link
Contributor

ef4 commented Sep 23, 2024

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
Copy link
Contributor

ef4 commented Sep 23, 2024

I wrote up more explanation in #2120 (comment) 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
Copy link

basz commented Sep 23, 2024

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

@mansona
Copy link
Member

mansona commented Sep 24, 2024

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
Copy link

basz commented Sep 24, 2024

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

@ef4
Copy link
Contributor

ef4 commented Sep 24, 2024

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).

@simonihmig
Copy link
Collaborator Author

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.

I don't want to argue over whether publishing JS vs. TS is better, since I do think you are probably right. But also I wouldn't want to spend time on making a v1 addon pre-compile TS, given that most of our addons are v2 already, and the effort migrating to v2 is probably better spent where this is not the case.

However, I don't think you are right about what has been recommended for v1 addons, or can you point me to a place where this has been recommended? ember-cli-typescript's addon docs say the opposite:

When you publish an addon written in TypeScript, the .ts files will be consumed and transpiled by Babel as part of building the host application

Even though you publish the source .ts files [...]

And ts:precompile was not about transpiling TS to JS, it was only about generating d.ts:

In order for your addon's users to benefit from type information from your addon, you need to put .d.ts declaration files at the location on disk where the compiler expects to find them. This addon provides two commands to help with that: ember ts:precompile and ember ts:clean [...] The ts:precompile command will populate the overall structure of your package with .d.ts files

@simonihmig
Copy link
Collaborator Author

Should be fixed by #2120

@ef4
Copy link
Contributor

ef4 commented Oct 2, 2024

And ts:precompile was not about transpiling TS to JS, it was only about generating d.ts:

Oh, I was mistaken then, I thought it also published the .js.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants