Skip to content

fix: Don't include 'import type' in JS output #178

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

Closed

Conversation

afiedler
Copy link

I ran into an issue where type imports were getting output in Javascript files. For example:

import type { A as ATypeOnly } from "./dir/src-file"

would be included in the .js file created by the Typescript compiler.

This PR excludes all type imports (node.importClause.isTypeOnly) from the output unless the output file is a declaration file.

@nonara
Copy link
Collaborator

nonara commented Aug 16, 2023

Thanks for the report and for contributing a fix!

That behaviour should already be covered here:

We had to copy most of that logic from the TS compiler. This is a place in our code where we have to catch up if TS changes anything fundamentally about that logic.

If that part isn't currently working, my guess is something has changed in the compiler. If you're interested in doing a bit of debugging, I'd take a look at that part of our code and see why it's not triggering.

If you look at the corresponding code in the TypeScript compiler, it should be evident what's different. My guess is it has something to do with changes to the factory method parameters.

I'll hopefully be able to have a look this weekend otherwise.

@pkerschbaum
Copy link

pkerschbaum commented Oct 7, 2023

I created a minimal reproduction repo for this issue: https://github.com/pkerschbaum/issue-typescript-transform-paths-type-is-included-in-js-output

Edit: I tried the fix of this PR and it did not change anything. Maybe because my reproduction repo has the problem with type only imports of named imports, while this PR exists to fix import type in JS output.

@JimmyDaddy
Copy link

Hello, does this repo still in maintain?

@nonara
Copy link
Collaborator

nonara commented Nov 8, 2023

@JimmyDaddy Yes. Running two companies, so I've been swamped. I have a week set off to catch up on this and all other OSS stuff coming up very soon.

@Brainfcker929
Copy link

I have the same bug.
Will this be merged?

Copy link
Member

@danielpza danielpza left a comment

Choose a reason for hiding this comment

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

LGTM

@nonara
Copy link
Collaborator

nonara commented Feb 16, 2024

Hey. Sorry. We should be fixing the elision part to match TS changes.

The changes here don't address it in the correct way or place.

I know I need to catch up on this. I'll check on this Sunday

@nonara
Copy link
Collaborator

nonara commented Feb 20, 2024

🎉 Fixed in the latest release — v3.4.7

Breakdown

The OP PR indicated that type-only import/export declarations were not working.

import type { MyType } '#a'

This would have been a regression, but it was not reproducible locally. However, if what was occurring was related to this library, it was likely due to OP using one of the new TypeScript v5+ compiler options.

The second issue mentioned by @pkerschbaum was separate from the one in this PR (Thank you for the repro!)

That issue was tied to the fact that type-only import/export specifiers is a new feature in v5, which needed to be covered in our elision code.

import { type SomeType } from "#a";

Ultimately, there was a bit that needed to be done to sync up with several new features in compiler options and syntax, but we should be fully covered now.

Better yet, we won't have to replicate elision functionality any more, as the TS team fixed the upstream issue! 🎉 It's merged, so I expect this will be included in the upcoming release.

This should make this library much more stable against new TS updates and easier to adjust when changes occur.

Full detail of issue:

Note on other issues

Most other open issues all center around the same thing, which is the challenging problem of mirroring node resolution strategy with our particular flavour of nuance. There is a new major version which has most of the logic written.

I'm working on getting that finally finished so we can be back to zero-issues, but the current state should work for all but those edge cases.

@nonara nonara closed this Feb 20, 2024
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.

6 participants