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 ts import issue with fcct #31

Closed
wants to merge 4 commits into from
Closed

Conversation

patricklx
Copy link
Contributor

No description provided.

`@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.
@patricklx patricklx changed the title wip attempt to fix ts import issue with fcct Oct 26, 2023
@NullVoxPopuli
Copy link
Contributor

Firstly, I really appreciate you trying to fix this problem!!!

however, I don't think we want to go down this route -- "two wrongs don't make a right" -- @ef4 is working on fixing the babel typescript plugin.

@patricklx patricklx changed the title attempt to fix ts import issue with fcct fix ts import issue with fcct Oct 27, 2023
@patricklx patricklx marked this pull request as ready for review October 27, 2023 09:13
@patricklx
Copy link
Contributor Author

patricklx commented Oct 27, 2023

@ef4 @NullVoxPopuli i know there is the attempt to fix it in typescript-transform, but think I have a good fix now. pre runs before any visitor, so it works independent of the order.
I do not think there is a good fix to do in typescript-transform, as it needs to to the removals before other plugins, otherwise amd transform etc, will convert the type imports and then it will be difficult to remove them again by typescript-transform.

@patricklx
Copy link
Contributor Author

The only downside is that it also does not remove unused type imports.
If they are annotated with type it works correctly

@ef4
Copy link
Contributor

ef4 commented Oct 27, 2023

I agree that we might need to do our own pre hack if fixes in typescript-transform turn out to be hard to land. And yeah, I had forgotten how annoying amd-transform is for these situations (embroider doesn't use it, but we still need to care about classic builds that do).

I think we can probably extend the ideas here to be more targeted though. For example:

  1. pre could capture a list of imports.
  2. When we're processing each template, we already know what names it needs. For each name:
    • if the name is not in scope
      • and the name was in the original list of imports
        • then we re-insert an import for that name.

This would allow all the normal removal of imports to take its usual course, and limit our changes to precisely the names that we know we need that other plugins can't know that we need.

@patricklx
Copy link
Contributor Author

patricklx commented Oct 27, 2023

I was also trying that first. But reinsert might be to late, since amd transform did already run. I thought dev mode doesn't do template analysis?
On the amd transform. I meant it in general, not specific to ember.
But we can remove unused specifiers ourselves. It's only about unused imports ( with specifiers).
At the end we can identify which are actually used. (if templates are actually analysed)

ef4 added a commit that referenced 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

ef4 commented Nov 1, 2023

I continued this branch in #32.

@ef4 ef4 closed this Nov 1, 2023
ef4 added a commit that referenced 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants