-
Notifications
You must be signed in to change notification settings - Fork 71
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
support newer moduleResolution
kinds, update build to TS 5.x
#453
Conversation
module: 'ES2022'
, update moduleResolution
default to node16
module: 'ES2022'
, update moduleResolution
default to node16
module: 'ES2022'
, update moduleResolution
default to node16
, update to to TS 5.x
module: 'ES2022'
, update moduleResolution
default to node16
, update to to TS 5.xmodule: 'ES2022'
, update moduleResolution
default to node16
, update build to TS 5.x
module: 'ES2022'
, update moduleResolution
default to node16
, update build to TS 5.xmoduleResolution
default to node16
, update build to TS 5.x
moduleResolution
default to node16
, update build to TS 5.xmoduleResolution
overrides, update build to TS 5.x
moduleResolution
overrides, update build to TS 5.xmoduleResolution
overrides, update build to TS 5.x
moduleResolution
overrides, update build to TS 5.xmoduleResolution
kinds, update build to TS 5.x
src/get-options-overrides.ts
Outdated
case tsModule.ModuleResolutionKind.Bundler: | ||
default: | ||
overrides.moduleResolution = tsModule.ModuleResolutionKind.Node10; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm gonna need to review this in a lot more detail (need to catch up on TS changes since 5.x hit GA and refresh myself on this codebase), but I mentioned mjs
support and bundler
in #434 (comment).
bundler
actually seemed the most appropriate at the time (since Rollup is a bundler) for Node16
+, particularly due to the file extension issues. That's actually why I didn't implement support for mjs
/ cjs
earlier because the file extension requirement could break many things (that and no user requested it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this basically lowest effort to let users use more moduleResolution values without figuring out exactly what the differences are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I think bundler
should not be overwritten either. With that change and README modifications about this, I think we could approve and merge this in.
Without tests though, we won't be able to confirm some of the behavior of node16
/ nodeNext
(particularly around file extensions) -- it is possible we may need to override those two to bundler
as well
Co-authored-by: Anton Gilgur <4970083+agilgur5@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mostly LGTM now, just left a tiny comment style comment. we'll ofc iterate on the docs as we test more and get feedback
Co-authored-by: Anton Gilgur <4970083+agilgur5@users.noreply.github.com>
@PetarKirov this PR only changed the There are potentially additional changes needed for the If you need |
This doesn't work with either |
Summary
Fixes #437
Details