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

fix gts ts imports issue #34

Merged
merged 10 commits into from
Apr 12, 2024
Merged

fix gts ts imports issue #34

merged 10 commits into from
Apr 12, 2024

Conversation

patricklx
Copy link
Contributor

@patricklx patricklx commented Mar 10, 2024

run the processing in pre to ensure we have the imports before other plugin runs.

fixes embroider-build/ember-auto-import#611

src/plugin.ts Outdated
@@ -194,6 +174,15 @@ export function makePlugin<EnvSpecificOptions>(loadOptions: (opts: EnvSpecificOp
},
},

Identifier(path: NodePath<t.Identifier>) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this ensures that newly added identifiers do actually reference the import binding. Otherwise ts plugin will remove the import

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this identifiers are also added by babel import util

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only a problem because we do this in pre

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is running in pre as well. So it only affects identifiers added during ast transform

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i could also have my own custom plugins that do some babel ast changes during template transform

@patricklx patricklx closed this Mar 18, 2024
@patricklx patricklx reopened this Mar 18, 2024
@ef4
Copy link
Contributor

ef4 commented Apr 2, 2024

Can we add a test that runs our plugin and babel-plugin-transform-typescript together so that we don't regress on this bug?

@patricklx
Copy link
Contributor Author

patricklx commented Apr 2, 2024

Can we add a test that runs our plugin and babel-plugin-transform-typescript together so that we don't regress on this bug?

We have it already

it('interoperates correctly with @babel/plugin-transform-typescript when handling locals with wire target', function () {

We probably would need another plugin that wants to use the imports.

@NullVoxPopuli
Copy link
Sponsor Contributor

We have it already

it wasn't failing? 😅

@patricklx
Copy link
Contributor Author

We have it already

it wasn't failing? 😅

No, because it's not a problem of ts plugin + this plugin. But a problem of
ts plugin + other plugin+ this plugin.

Updated the description

@ef4
Copy link
Contributor

ef4 commented Apr 2, 2024

Let me rephrase my question: can we add a test they would fail without these changes?

@patricklx
Copy link
Contributor Author

@ef4 added a test

@ef4
Copy link
Contributor

ef4 commented Apr 12, 2024

Thanks, I found a way to use babel's scope.crawl() to update bindings for only the sub-trees of the AST that we're altering.

@ef4 ef4 merged commit cd3ce26 into emberjs:main Apr 12, 2024
4 checks passed
@ef4 ef4 added the bug Something isn't working label Apr 17, 2024
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.

issue with gts/gjs if ts transform is enabled
3 participants