Skip to content

Preserve used imports when interoperating with transform-typescript#32

Merged
ef4 merged 5 commits intomainfrom
preserve-used-imports
Nov 1, 2023
Merged

Preserve used imports when interoperating with transform-typescript#32
ef4 merged 5 commits intomainfrom
preserve-used-imports

Conversation

@ef4
Copy link
Copy Markdown
Contributor

@ef4 ef4 commented Nov 1, 2023

Builds off #31 to fix #30.

Instead of keeping everything as in #31 (which is not safe in general), we use the pre to take a snapshot of available imports and then only when we discover that a template wants to use a name that is not in scope do we check if it was in the original set of available imports and reintroduce an import for it.

ef4 and others added 4 commits August 30, 2023 20:21
`@babel/plugin-transform-typescript` removes unused imports because it assumes they're types (yuck). That's how TS behaves so it's at least understandable.

However, it doesn't seem to respect the fact that our plugin is *emitting* code that *will* use the imports.

This adds a failing test.
@ef4 ef4 mentioned this pull request Nov 1, 2023
Builds off #31 to fix #30.

Instead of keeping *everything* as in #31 (which is not safe in general), we use the `pre` to take a snapshot of available imports and then only when we discover that a template wants to use a name that is not in scope do we check if it was in the original set of available imports and reintroduce an import for it.
@ef4 ef4 force-pushed the preserve-used-imports branch from 9d789ed to 3d9f9d4 Compare November 1, 2023 19:59
@ef4
Copy link
Copy Markdown
Contributor Author

ef4 commented Nov 1, 2023

I confirmed locally that this PR fixes ember-cli/ember-template-imports#211

@ef4 ef4 merged commit 81f5438 into main Nov 1, 2023
@delete-merged-branch delete-merged-branch bot deleted the preserve-used-imports branch November 1, 2023 20:17
@ef4 ef4 added the bug Something isn't working label Nov 1, 2023
@ef4 ef4 changed the title Preserve used imports Preserve used imports when interoperating with transform-typescript Nov 1, 2023
@patricklx
Copy link
Copy Markdown
Contributor

patricklx commented Nov 1, 2023

Does this also work with amd transform? Would be good to have a test for it too .

Btw, thanks for basing this on my pr. Much appreciated. if this works well, also with AMD transform, then this is better

@ef4
Copy link
Copy Markdown
Contributor Author

ef4 commented Nov 1, 2023

This PR didn't change anything about the timing of when we add imports. We were already adding imports in a bunch of places within the expression-level visitors.

@patricklx
Copy link
Copy Markdown
Contributor

I also saw now that amd transform runs an program:exit

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.

2 participants