Support import paths with .gts extensions #620
Conversation
lukemelia
force-pushed
the
gts-imports
branch
3 times, most recently
from
3c5a721
to
ec568f4
Compare
| @@ -28,7 +28,7 @@ jobs: | |||
| - name: Install Node | |||
| uses: actions/setup-node@v3 | |||
| with: | |||
| node-version: 14 | |||
| node-version: 16 | |||
There was a problem hiding this comment.
why did node change? was node 14 supported?
There was a problem hiding this comment.
The project's volta config specified 16 but CI was running as 14. This started being a problem when I added a dependency on @typescript-eslint/eslint-plugin to the ts-template-imports-app package, which requires node 16+
| @@ -58,8 +58,15 @@ export default class DocumentCache { | |||
| ); | |||
| } | |||
|
|
|||
| private gtsAwareExtName(filename: string): string { | |||
| if (filename.endsWith('.gts.ts')) { | |||
There was a problem hiding this comment.
This is the extension that TypeScript tries if you import a .gts module since it has no intrinsic understanding of the .gts extension. We're intercepting it to map it to the actual file.
There was a problem hiding this comment.
This is worth a comment in the source because it will definitely look weird to future readers.
| ); | ||
| let originalStart = node.getStart(); | ||
| let originalLength = node.getEnd() - originalStart; | ||
| const printer = ts.createPrinter(); |
There was a problem hiding this comment.
do we need to create a new printer each time this function is called?
There was a problem hiding this comment.
👍 Will improve that
| const Bang: TOC<{ Args: { times: number } }> = <template> | ||
| {{repeat '!' @times}} | ||
| </template> | ||
|
|
||
| declare module '@glint/environment-ember-loose/registry' { |
There was a problem hiding this comment.
why was this added? it's only needed for loose mode
There was a problem hiding this comment.
There was a problem hiding this comment.
I thought it would be a good idea to incorporate loose mode to make sure we have decent coverage for this scenario since in today's apps using .gts files, they will still have route templates.
| return params[0].repeat(params[1]); | ||
| } | ||
|
|
||
| const repeatHelper = helper(repeat); |
There was a problem hiding this comment.
why wrap in helper?
As of ember-source@4.5, we don't need helper, and can "just" export the functions (and change the signature to be a normal function as it once before it was moved in to this file)
| @@ -0,0 +1,3 @@ | |||
| <div class="main"> | |||
| <Greeting @target="Earthling" /> | |||
| @@ -4,7 +4,17 @@ | |||
| "private": true, | |||
| "license": "MIT", | |||
| "scripts": { | |||
| "test": "glint" | |||
| "test": "npm-run-all test:*", | |||
| "test:typecheck": "glint", | |||
There was a problem hiding this comment.
Actually, I did this because the other test apps are running both of these.
lukemelia
force-pushed
the
gts-imports
branch
3 times, most recently
from
ccd40c5
to
0967bb9
Compare
lukemelia
force-pushed
the
gts-imports
branch
from
0967bb9
to
3a634c7
Compare
|
Updates made! Thanks for the feedback @NullVoxPopuli |
| @@ -58,8 +58,15 @@ export default class DocumentCache { | |||
| ); | |||
| } | |||
|
|
|||
| private gtsAwareExtName(filename: string): string { | |||
| if (filename.endsWith('.gts.ts')) { | |||
There was a problem hiding this comment.
This is worth a comment in the source because it will definitely look weird to future readers.
There was a problem hiding this comment.
Sorry, I've been on vacation and disconnected this week, and I'm catching up on my phone on the train back now.
I'll dive deeper when I have computer access again, but please hold on merging this. I don't think the transform layer is likely the right place to be solving this, which would explain why it's been so painful.
|
I've opened a PR with an alternative approach to this problem in #621 @lukemelia I want to apologize for the miscommunication on this and the time/effort it's cost you—it's 100% my fault. I'd had a separate conversation in Discord about what would need to happen to fix this, and then on #618 I got sidetracked by some of the meta-questions being asked and didn't focus properly on the fix you were suggesting there to realize that you hadn't been a part of or seen that original conversation. Thank you very much for your work on this, and I'm sorry for pointing you in the wrong direction! |
|
No worries, @dfreeman. Thanks for resetting us onto the right path. |
Note that using imports with extensions requires TypeScript 5 and enabling the setting to support importing files with typescript extensions ("allowImportingTsExtensions").
Fixes #618