Skip to content

module: fix strip-types interaction with detect-module#54164

Merged
nodejs-github-bot merged 1 commit intonodejs:mainfrom
marco-ippolito:fix/interaction-with-detect-module
Aug 3, 2024
Merged

module: fix strip-types interaction with detect-module#54164
nodejs-github-bot merged 1 commit intonodejs:mainfrom
marco-ippolito:fix/interaction-with-detect-module

Conversation

@marco-ippolito
Copy link
Member

@marco-ippolito marco-ippolito commented Aug 1, 2024 •
edited
Loading

This PR fixes some weird interaction between experimental-strip-types and the unflagged experimental-detect-module

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 1, 2024

Review requested:

  • @nodejs/loaders
  • @nodejs/typescript

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Aug 1, 2024
@marco-ippolito marco-ippolito mentioned this pull request Aug 1, 2024
Merged
@marco-ippolito marco-ippolito added the strip-types Issues or PRs related to strip-types support label Aug 1, 2024
anonrig
anonrig reviewed Aug 1, 2024
View reviewed changes
@marco-ippolito marco-ippolito force-pushed the fix/interaction-with-detect-module branch 2 times, most recently from c0d35bd to 8565004 Compare August 1, 2024 17:27
anonrig
anonrig reviewed Aug 1, 2024
View reviewed changes
@marco-ippolito marco-ippolito force-pushed the fix/interaction-with-detect-module branch 4 times, most recently from 485cc4c to e23011d Compare August 1, 2024 17:50
aduh95
aduh95 reviewed Aug 1, 2024
View reviewed changes
@marco-ippolito marco-ippolito force-pushed the fix/interaction-with-detect-module branch from e23011d to e13d328 Compare August 1, 2024 18:24
@marco-ippolito marco-ippolito force-pushed the fix/interaction-with-detect-module branch from e13d328 to 1701a4b Compare August 1, 2024 18:26
@marco-ippolito marco-ippolito added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 1, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 1, 2024
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 1, 2024

CI: https://ci.nodejs.org/job/node-test-pull-request/60805/

@codecov
Copy link

codecov bot commented Aug 1, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 90.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 87.06%. Comparing base (00c0644) to head (1701a4b).
Report is 521 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/modules/esm/get_format.js 50.00% 1 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##             main   #54164   +/-   ##
=======================================
  Coverage   87.05%   87.06%           
=======================================
  Files         643      643           
  Lines      181588   181594    +6     
  Branches    34896    34895    -1     
=======================================
+ Hits       158088   158108   +20     
+ Misses      16771    16761   -10     
+ Partials     6729     6725    -4
Files with missing lines Coverage Δ
lib/internal/modules/helpers.js 97.74% <100.00%> (+0.03%) ⬆️
lib/internal/modules/run_main.js 97.28% <100.00%> (ø)
lib/internal/modules/esm/get_format.js 90.36% <50.00%> (+2.05%) ⬆️

... and 24 files with indirect coverage changes

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 2, 2024

CI: https://ci.nodejs.org/job/node-test-pull-request/60816/

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 2, 2024

CI: https://ci.nodejs.org/job/node-test-pull-request/60817/

@marco-ippolito marco-ippolito added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 2, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 2, 2024
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 2, 2024

CI: https://ci.nodejs.org/job/node-test-pull-request/60818/

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 2, 2024

CI: https://ci.nodejs.org/job/node-test-pull-request/60823/

@marco-ippolito
Copy link
Member Author

marco-ippolito commented Aug 2, 2024

@nodejs/loaders can I get a review on this one pls 🙏🏼

jakecastelli
jakecastelli reviewed Aug 2, 2024
View reviewed changes
test/fixtures/typescript/ts/test-import-fs.ts
@@ -0,0 +1,2 @@
import fs from 'fs';
Copy link
Member

@jakecastelli jakecastelli Aug 2, 2024

Choose a reason for hiding this comment

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

nit: in other typescript fixtures we have node: prefix

mcollina
mcollina approved these changes Aug 2, 2024
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@marco-ippolito marco-ippolito added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Aug 2, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 3, 2024
@nodejs-github-bot nodejs-github-bot merged commit 5d6c76a into nodejs:main Aug 3, 2024
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 3, 2024

Landed in 5d6c76a

@marco-ippolito marco-ippolito added backport-open-v20.x Indicate that the PR has an open backport backport-open-v22.x Indicate that the PR has an open backport and removed backport-open-v20.x Indicate that the PR has an open backport labels Aug 5, 2024
marco-ippolito added a commit to marco-ippolito/node that referenced this pull request Aug 5, 2024
@marco-ippolito
module: fix strip-types interaction with detect-module
23d5d60 
PR-URL: nodejs#54164
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@marco-ippolito marco-ippolito mentioned this pull request Aug 5, 2024
Closed
marco-ippolito added a commit to marco-ippolito/node that referenced this pull request Aug 5, 2024
@marco-ippolito
module: fix strip-types interaction with detect-module
01631f3 
PR-URL: nodejs#54164
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
marco-ippolito added a commit that referenced this pull request Aug 5, 2024
@marco-ippolito
module: fix strip-types interaction with detect-module
c20e841 
PR-URL: #54164
Backport-PR-URL: #54211
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@targos targos added dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. backported-to-v22.x PRs backported to the v22.x-staging branch. and removed backport-open-v22.x Indicate that the PR has an open backport labels Sep 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@targos targos targos left review comments

@aduh95 aduh95 aduh95 left review comments

@mcollina mcollina mcollina approved these changes

@ShogunPanda ShogunPanda ShogunPanda approved these changes

@anonrig anonrig anonrig approved these changes

@himself65 himself65 himself65 approved these changes

@jakecastelli jakecastelli jakecastelli approved these changes

+1 more reviewer

@khaosdoctor khaosdoctor khaosdoctor approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backported-to-v22.x PRs backported to the v22.x-staging branch. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. strip-types Issues or PRs related to strip-types support

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@marco-ippolito @nodejs-github-bot @mcollina @ShogunPanda @anonrig @targos @khaosdoctor @himself65 @aduh95 @jakecastelli