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

Preserve used imports when interoperating with transform-typescript #32

Merged
merged 5 commits into from
Nov 1, 2023

Conversation

ef4
Copy link
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
Copy link
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
4 checks passed
@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
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
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
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