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

Transform type exports to "export type" in order to comply with isolatedModules #278

Merged
merged 7 commits into from
Aug 4, 2023

Conversation

mithodin
Copy link
Contributor

@mithodin mithodin commented Aug 4, 2023

Fixes #276

When the TypeScript option isolatedModules is used, re-exported types must be exported as `export type { ... }` or `export { type ... }`. This fixup determines the kind of export (type or anything else) and adapts the export statement accordingly.
Copy link
Owner

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

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

This looks like a pragmatic solution that works, great job!

src/transform/index.ts Show resolved Hide resolved
@mithodin
Copy link
Contributor Author

mithodin commented Aug 4, 2023

@Swatinem it seems that export { type T } is only supported since Typescript 4.5. Would you be willing to raise the minimum compatible ts version to 4.5, or would you prefer we split type exports from value exports? Going to 4.5 also fixes the issue with

TypeError: Cannot read properties of null (reading 'deoptimizePath')

that we see in the ci logs.

@mithodin
Copy link
Contributor Author

mithodin commented Aug 4, 2023

Also, support for node 14 can probably be dropped since it is end-of-life since May of this year.

@mithodin
Copy link
Contributor Author

mithodin commented Aug 4, 2023

This would probably mean a 6.0.0 release, right?

Copy link
Owner

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

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

I do see both sides of bumping version requirements.

From a consumer point of view, bumping versions in most cases is as easy as a oneliner. If it isn’t, and someone is stuck on older versions, it does not make a difference being stuck on an older version of this plugin either.

From a maintainers point of view, lowering the maintenance burden by not having to support older versions is definitely a plus. Given than backwards compatibility to older Rollup versions was already broken by accident recently, might as well bump that up as well in accordance with that. For the Typescript version itself, not sure what a reasonable version requirement there would be. 4.5 sounds reasonable, as that was released end of 2021. Might as well bump higher, as I really don’t see a point supporting TS older than a year.

package.json Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@Swatinem Swatinem merged commit 1b228d0 into Swatinem:master Aug 4, 2023
4 checks passed
@mithodin mithodin deleted the master branch August 4, 2023 12:34
@mithodin
Copy link
Contributor Author

mithodin commented Aug 7, 2023

@Swatinem thanks for the merge :-) Do you have any plans yet for when to do a release?

@mithodin
Copy link
Contributor Author

Hi @Swatinem
It would be really cool if you could release this so we can actually get our issue fixed :-) Could you please do it?

@Swatinem
Copy link
Owner

Done, sorry for the delay.

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.

Invalid exports when using isolatedModules: true
3 participants