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

feat(typescript): support 4.7 nodenext #1194

Merged

Conversation

perrin4869
Copy link
Contributor

@perrin4869 perrin4869 commented May 26, 2022

Rollup Plugin Name: typescript

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

List any relevant issue numbers:

Description

Add support for the new module: nodenext/node16 typescript options in 4.7. This is breaking because it will now require typescript >= 4.7. I tested that this works in my environment via patch-package, but there are broken tests, etc. If one of the maintainers can provide pointers towards getting this ready I will be highly grateful!

@perrin4869 perrin4869 changed the title Feature/typescript/support nodenext feat(typescript): support 4.7 nodenext May 26, 2022
@perrin4869 perrin4869 marked this pull request as ready for review June 7, 2022 05:26
@perrin4869
Copy link
Contributor Author

@shellscape @lukastaegert

OK I finally got around to deep dive into this issue and iron out the details.
Originally I thought that this would be a breaking change and require typescript 4.7, but thinking more carefully, I think this will work perfectly fine with older typescript versions as it has thus far.
Took inspiration from @cspotcode's work on TypeStrong/ts-node#1778, couldn't have figured this all out without his help!
I added some tests to ensure typescript is resolving to the expected module type (cjs or esnext), and hopefully didn't introduce any regressions in the process. The previous tests are all passing as usual.

Copy link
Collaborator

@shellscape shellscape left a comment

Choose a reason for hiding this comment

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

Thanks for the time you put into this. We'll need one more review to merge this, I'll tag a few folks.

Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

Looks very promising. Some questions about globbing.

packages/typescript/src/index.ts Outdated Show resolved Hide resolved
packages/typescript/src/index.ts Outdated Show resolved Hide resolved
Copy link

@Janmodj Janmodj left a comment

Choose a reason for hiding this comment

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

Copy link

@Janmodj Janmodj left a comment

Choose a reason for hiding this comment

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

@Mister-Hope
Copy link

Any update about this PR?

@perrin4869
Copy link
Contributor Author

I could fix the conflicts but I'll need someone to merge it 😅

@shellscape
Copy link
Collaborator

I could fix the conflicts but I'll need someone to merge it

@perrin4869 please do, then ping me directly if you don't see action within 24 hours.

@perrin4869
Copy link
Contributor Author

Done! 😄

packages/typescript/src/index.ts Outdated Show resolved Hide resolved
@perrin4869 perrin4869 requested review from NotWoods and removed request for guybedford, tjenkinson and lukastaegert September 4, 2022 16:40
@shellscape shellscape merged commit 780f2e7 into rollup:master Sep 6, 2022
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.

None yet

7 participants