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

Update config templates to properly handle .gts files #190

Merged
merged 1 commit into from
Sep 6, 2023

Conversation

lukemelia
Copy link
Contributor

This is an attempt to illustrate a problem I had doing a v1 -> v2 addon migration for an addon that includes .gts components.

The new test fails with the following output:

prepare: > concurrently 'npm:build:*'
.. prepare: [js] > [email protected] build:js
.. prepare: [js] > rollup --config
.. prepare: [js] 
.. prepare: [js]  → dist...
.. prepare: [js] [!] (plugin babel) SyntaxError: /private/var/folders/lf/n0vr44g54f70hr2sh9w5hmq00000gn/T/v2-addon-blueprint--52OYD5/my-addon/my-addon/src/components/template-import.gts: Unexpected reserved word 'interface'. (7:0)
.. prepare: [js] 
.. prepare: [js]    5 | import { action } from '@ember/object';
.. prepare: [js]    6 | import { fn } from '@ember/helper';
.. prepare: [js] >  7 | interface Signature {
.. prepare: [js]      | ^
.. prepare: [js]    8 |     Element: HTMLDivElement;
.. prepare: [js]    9 |     Args: {
.. prepare: [js]   10 |         saying?: string;
.. prepare: [js] src/components/template-import.gts (7:0)
.. prepare: [js]     at instantiate (/private/var/folders/lf/n0vr44g54f70hr2sh9w5hmq00000gn/T/v2-addon-blueprint--52OYD5/my-addon/node_modules/.pnpm/@[email protected]/node_modules/@babel/parser/src/parse-error/credentials.ts:62:21)
.. prepare: [js]     at toParseError (/private/var/folders/lf/n0vr44g54f70hr2sh9w5hmq00000gn/T/v2-addon-blueprint--52OYD5/my-addon/node_modules/.pnpm/@[email protected]/node_modules/@babel/parser/src/parse-error.ts:60:12)
.. prepare: [js]     at Parser.raise (/private/var/folders/lf/n0vr44g54f70hr2sh9w5hmq00000gn/T/v2-addon-blueprint--52OYD5/my-addon/node_modules/.pnpm/@[email protected]/node_modules/@babel/parser/src/tokenizer/index.ts:1487:19)
.. prepare: [js]     at Parser.checkReservedWord (/private/var/folders/lf/n0vr44g54f70hr2sh9w5hmq00000gn/T/v2-addon-blueprint--52OYD5/my-addon/node_modules/.pnpm/@[email protected]/node_modules/@babel/parser/src/parser/expression.ts:2808:12)
.. prepare: [js]     at Parser.parseIdentifierName (/private/var/folders/lf/n0vr44g54f70hr2sh9w5hmq00000gn/T/v2-addon-blueprint--52OYD5/my-addon/node_modules/.pnpm/@[email protected]/node_modules/@babel/parser/src/parser/expression.ts:2769:12)
.. prepare: [js]     at Parser.parseIdentifier (/private/var/folders/lf/n0vr44g54f70hr2sh9w5hmq00000gn/T/v2-addon-blueprint--52OYD5/my-addon/node_modules/.pnpm/@[email protected]/node_modules/@babel/parser/src/parser/expression.ts:2734:23)
.. prepare: [js]     at Parser.parseExprAtom (/private/var/folders/lf/n0vr44g54f70hr2sh9w5hmq00000gn/T/v2-addon-blueprint--52OYD5/my-addon/node_modules/.pnpm/@[email protected]/node_modules/@babel/parser/src/parser/expression.ts:1296:27)
.. prepare: [js]     at Parser.parseExprSubscripts (/private/var/folders/lf/n0vr44g54f70hr2sh9w5hmq00000gn/T/v2-addon-blueprint--52OYD5/my-addon/node_modules/.pnpm/@[email protected]/node_modules/@babel/parser/src/parser/expression.ts:710:23)
.. prepare: [js]     at Parser.parseUpdate (/private/var/folders/lf/n0vr44g54f70hr2sh9w5hmq00000gn/T/v2-addon-blueprint--52OYD5/my-addon/node_modules/.pnpm/@[email protected]/node_modules/@babel/parser/src/parser/expression.ts:687:21)
.. prepare: [js]     at Parser.parseMaybeUnary (/private/var/folders/lf/n0vr44g54f70hr2sh9w5hmq00000gn/T/v2-addon-blueprint--52OYD5/my-addon/node_modules/.pnpm/@[email protected]/node_modules/@babel/parser/src/parser/expression.ts:649:23)
.. prepare: [js] 
.. prepare: [js] npm run build:js exited with code 1

@ef4
Copy link
Contributor

ef4 commented Aug 29, 2023

The babel config needs:

"presets": [["@babel/preset-typescript", { "allExtensions": true }]],

Personally, I don't think we should bother with @babel/preset-typescript anyway. All it does is add @babel/plugin-transform-typescript, which we could just add directly, avoiding its irrelevant opinions about file extensions.

@lukemelia
Copy link
Contributor Author

Thanks @ef4. I made this change and made it a bit further. Here's the new error:

not ok 3 Chrome 116.0 - [43 ms] - Rendering | template-import: it renders
    ---
        stack: >
            ReferenceError: TemplateOnly is not defined
                at Object.scope (webpack://__ember_auto_import__/../my-addon/dist/components/template-import.js?:22:545)
                at meta (http://localhost:7357/assets/vendor.js:38992:86)
                at compilable (http://localhost:7357/assets/vendor.js:40602:51)
                at TemplateImpl.asLayout (http://localhost:7357/assets/vendor.js:41275:28)
                at ConstantsImpl.resolvedComponent (http://localhost:7357/assets/vendor.js:41542:53)
                at resolveComponent (http://localhost:7357/assets/vendor.js:38424:22)
                at encodeOp (http://localhost:7357/assets/vendor.js:40708:18)
                at pushOp (http://localhost:7357/assets/vendor.js:40632:7)
                at http://localhost:7357/assets/vendor.js:40245:7
                at Compilers.compile (http://localhost:7357/assets/vendor.js:38699:7)
        message: >
            global failure: ReferenceError: TemplateOnly is not defined
        negative: >
            false
        browser log: |
            {"type":"error","text":"\n\nError occurred:\n\n- While rendering:\n  -top-level\n    application\n      index\n\n"}
            {"type":"error","text":"\n\nError occurred:\n\n\n\n"}
    ...

My guess is that the import of TemplateOnly is being stripped out as unused, but I'm not sure why or by what.

@ef4
Copy link
Contributor

ef4 commented Aug 30, 2023

Diagnosed this as emberjs/babel-plugin-ember-template-compilation#30.

You can avoid the problem by configuring onlyRemoveTypeImports: true on @babel/preset-typescript.

The next issue you'll hit after that is:

RollupError: Could not resolve "./another-gts" from "src/components/template-import.gts"

But that is because relative imports inside a v2 addon should have explicit file extensions (this is consistent with how node treats ES modules). So you need:

-import AnotherGts from './another-gts';
+import AnotherGts from './another-gts.gts';

Note that we don't error on a missing .js extension because rollup does have built-in support for automagically adding that one. But anything else nonstandard like .gts definitely needs an extension.

Also note that:

import TemplateOnly from './template-only';

is working because template-only components are automatically .js files, even when you've only authored the .hbs. You could also have said ./template-only.js. You cannot say ./template-only.hbs because that historically means a different thing (it means the "bare" template itself, not a component that uses that template).

@ef4
Copy link
Contributor

ef4 commented Aug 30, 2023

Stuff that needs fixing in the blueprint to close this issue:

  • set allExtensions
  • set onlyRemoveTypeImports
  • see if we can opt rollup into not automatically adding .js extensions to relative imports, because it creates an inconsistent teaching story.

- Adds a .gts component to typescript smoke test coverage
@lukemelia
Copy link
Contributor Author

@ef4 I updated the config, added the .gts extension to the import, and verified that tests pass. I did a little research into the rollup .js question and didn't get anywhere. I agree that this is seriously confusing though, and we should figure out a way to have a more consistent story.

@lukemelia lukemelia changed the title Add .gts component to typescript smoke test coverage Update config templates to properly handle .gts files Aug 31, 2023
@NullVoxPopuli
Copy link
Collaborator

image

@NullVoxPopuli NullVoxPopuli added the bug Something isn't working label Sep 6, 2023
@NullVoxPopuli NullVoxPopuli merged commit c7ad65d into embroider-build:main Sep 6, 2023
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.

3 participants