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): use Node16 module & moduleResolution when appropriate #2791

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

RomainMuller
Copy link
Contributor

Instead of using CommonJS and Node, apply the Node16 module and module resolution settings, which are preferred when targeting node >=16 as it allows dynamically choosing the correct algorithms based on file extension as well as package.json configuration.

Additionally, automatically configure target and lib based on the configured minimum node version (if present).


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

RomainMuller and others added 2 commits July 12, 2023 10:58
Instead of using `CommonJS` and `Node`, apply the `Node16` module and
module resolution settings, which are preferred when targeting node >=16
as it allows dynamically choosing the correct algorithms based on file
extension as well as package.json configuration.

Additionally, automatically configure `target` and `lib` based on the
configured minimum node version (if present).
Signed-off-by: github-actions <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Jul 12, 2023

Codecov Report

Merging #2791 (4f0ccec) into main (18c6058) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main    #2791      +/-   ##
==========================================
+ Coverage   96.39%   96.41%   +0.02%     
==========================================
  Files         181      181              
  Lines       33649    33741      +92     
  Branches     3099     3113      +14     
==========================================
+ Hits        32435    32531      +96     
+ Misses       1214     1210       -4     
Impacted Files Coverage Δ
src/github/pull-request-lint.ts 100.00% <100.00%> (ø)
src/javascript/typescript-config.ts 99.73% <100.00%> (+0.02%) ⬆️
src/typescript/typescript.ts 97.73% <100.00%> (+0.10%) ⬆️
src/util.ts 95.30% <100.00%> (+0.05%) ⬆️
test/util.ts 95.06% <100.00%> (+0.22%) ⬆️

... and 2 files with indirect coverage changes

src/javascript/typescript-config.ts Outdated Show resolved Hide resolved
src/javascript/typescript-config.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@mrgrain mrgrain changed the title chore: use Node16 module & moduleResolution when appropriate feat(typescript): use Node16 module & moduleResolution when appropriate Jul 12, 2023
@mrgrain
Copy link
Contributor

mrgrain commented Jul 12, 2023

Is this going to be a breaking change because the module field is now typed?

@RomainMuller
Copy link
Contributor Author

Is this going to be a breaking change because the module field is now typed?

Good question. I suspect possibly maybe? I can revert this to being string and replace the enum with a constants helper?

@mrgrain mrgrain changed the title feat(typescript): use Node16 module & moduleResolution when appropriate feat(typescript) use Node16 module & moduleResolution when appropriate Jul 17, 2023
@mrgrain mrgrain changed the title feat(typescript) use Node16 module & moduleResolution when appropriate feat(typescript): use Node16 module & moduleResolution when appropriate Jul 17, 2023
github-actions and others added 2 commits July 17, 2023 09:33
@mrgrain mrgrain changed the title feat(typescript): use Node16 module & moduleResolution when appropriate feat(typescript) use Node16 module & moduleResolution when appropriate Jul 17, 2023
@mrgrain mrgrain changed the title feat(typescript) use Node16 module & moduleResolution when appropriate feat(typescript): use Node16 module & moduleResolution when appropriate Jul 17, 2023
@doublecompile
Copy link

Side note: I determined the proper configuration to get Projen and Jest working correctly with a TypeScript project where the package.json has type set to module (i.e. ESM instead of CommonJS), check out this gist for an example .projenrc.ts.

@mrgrain
Copy link
Contributor

mrgrain commented Oct 22, 2023

Note to myself: https://github.com/microsoft/TypeScript/wiki/Node-Target-Mapping

jsii probably needs to update these as well

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

4 participants