search

LeDDGroup / typescript-transform-paths

Transforms module resolution paths using TypeScript path mapping and/or custom paths
MIT License
465 stars 22 forks source link

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

Closed afiedler closed 4 months ago

afiedler commented 11 months ago

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 commented 11 months ago

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 commented 9 months ago

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 commented 8 months ago

Hello, does this repo still in maintain?

nonara commented 8 months ago

@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 commented 5 months ago

I have the same bug. Will this be merged?

nonara commented 5 months ago

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 commented 4 months ago

🎉 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.