Skip to content

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

Merged
nodejs-github-bot merged 1 commit into
nodejs: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 into
nodejs:mainfrom
marco-ippolito:fix/interaction-with-detect-module

Conversation

@marco-ippolito

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

Copy link
Copy Markdown
Member

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

@nodejs-github-bot

nodejs-github-bot commented Aug 1, 2024

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/typescript

@nodejs-github-botnodejs-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-ippolitomarco-ippolito mentioned this pull request Aug 1, 2024
Merged
@marco-ippolitomarco-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
Comment threadlib/internal/modules/esm/get_format.js Outdated
@marco-ippolito
marco-ippolitoforce-pushed the fix/interaction-with-detect-module branch 2 times, most recently from c0d35bd to 8565004CompareAugust 1, 2024 17:27
anonrig
anonrig reviewed Aug 1, 2024
View reviewed changes
Comment threadlib/internal/modules/helpers.js Outdated
@marco-ippolito
marco-ippolitoforce-pushed the fix/interaction-with-detect-module branch 4 times, most recently from 485cc4c to e23011dCompareAugust 1, 2024 17:50
aduh95
aduh95 reviewed Aug 1, 2024
View reviewed changes
Comment threadtest/es-module/test-typescript-eval.mjs Outdated
@marco-ippolito
marco-ippolitoforce-pushed the fix/interaction-with-detect-module branch from e23011d to e13d328CompareAugust 1, 2024 18:24
@marco-ippolito
marco-ippolitoforce-pushed the fix/interaction-with-detect-module branch from e13d328 to 1701a4bCompareAugust 1, 2024 18:26
@marco-ippolitomarco-ippolito added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 1, 2024
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 1, 2024
@nodejs-github-bot

nodejs-github-bot commented Aug 1, 2024

Copy link
Copy Markdown
Collaborator

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

@codecov

codecovBot commented Aug 1, 2024•
edited
Loading

Copy link
Copy Markdown

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 linesPatch %Lines
lib/internal/modules/esm/get_format.js50.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 linesCoverage Δ
lib/internal/modules/helpers.js97.74% <100.00%> (+0.03%)⬆️
lib/internal/modules/run_main.js97.28% <100.00%> (ø)
lib/internal/modules/esm/get_format.js90.36% <50.00%> (+2.05%)⬆️

... and 24 files with indirect coverage changes

@nodejs-github-bot

nodejs-github-bot commented Aug 2, 2024

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Aug 2, 2024

Copy link
Copy Markdown
Collaborator

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

@marco-ippolitomarco-ippolito added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 2, 2024
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 2, 2024
@nodejs-github-bot

nodejs-github-bot commented Aug 2, 2024

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Aug 2, 2024

Copy link
Copy Markdown
Collaborator

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

@marco-ippolito

marco-ippolito commented Aug 2, 2024

Copy link
Copy Markdown
MemberAuthor

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

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

@jakecastellijakecastelliAug 2, 2024

Copy link
Copy Markdown
Member

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

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@marco-ippolitomarco-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-botnodejs-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:mainAug 3, 2024
@nodejs-github-bot

nodejs-github-bot commented Aug 3, 2024

Copy link
Copy Markdown
Collaborator

Landed in 5d6c76a

@marco-ippolitomarco-ippolito added backport-open-v20.x backport-open-v22.x Indicate that the PR has an open backport and removed backport-open-v20.x labels Aug 5, 2024
@marco-ippolitomarco-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>
@targostargos added dont-land-on-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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@targostargostargos left review comments
@aduh95aduh95aduh95 left review comments
@mcollinamcollinamcollina approved these changes
@ShogunPandaShogunPandaShogunPanda approved these changes
@anonriganonriganonrig approved these changes
@himself65himself65himself65 approved these changes
@jakecastellijakecastellijakecastelli approved these changes
+1 more reviewer
@khaosdoctorkhaosdoctorkhaosdoctor approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.backported-to-v22.xPRs backported to the v22.x-staging branch.lib / srcIssues and PRs related to general changes in the lib or src directory.needs-ciPRs that need a full CI run.strip-typesIssues 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