GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — module: fix strip-types interaction with detect-module by marco-ippolito · Pull Request #54164 · nodejs/node

Conversation

marco-ippolito commented Aug 1, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss and benjamingr reacted with thumbs up emoji

All reactions

👍 2 reactions

nodejs-github-bot commented Aug 1, 2024

Copy link

Copy Markdown

Collaborator

Review requested:

@nodejs/loaders
@nodejs/typescript

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nodejs-github-bot added lib / src

Issues and PRs involving general changes in the lib/ or src/ directories.

needs-ci PRs that need a full CI run. labels

Aug 1, 2024

marco-ippolito mentioned this pull request

Aug 1, 2024

v22.6.0 proposal #54123

Merged

marco-ippolito added the strip-types Issues and PRs related to TypeScript type stripping. label

Aug 1, 2024

anonrig reviewed

Aug 1, 2024

View reviewed changes

Comment thread

lib/internal/modules/esm/get_format.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

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 reviewed

Aug 1, 2024

View reviewed changes

Comment thread

lib/internal/modules/helpers.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

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 reviewed

Aug 1, 2024

View reviewed changes

Comment thread

test/es-module/test-typescript-eval.mjs Resolved

Uh oh!

There was an error while loading. Please reload this page.

marco-ippolito force-pushed the fix/interaction-with-detect-module branch from e23011d to e13d328 Compare

August 1, 2024 18:24


          module: fix strip-types interaction with detect-module

1701a4b

marco-ippolito force-pushed the fix/interaction-with-detect-module branch from e13d328 to 1701a4b Compare

August 1, 2024 18:26

marco-ippolito added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Aug 1, 2024

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Aug 1, 2024

nodejs-github-bot commented Aug 1, 2024

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

codecov Bot commented Aug 1, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

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 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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

khaosdoctor approved these changes

Aug 1, 2024

View reviewed changes

nodejs-github-bot commented Aug 2, 2024

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nodejs-github-bot commented Aug 2, 2024

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

marco-ippolito added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Aug 2, 2024

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Aug 2, 2024

nodejs-github-bot commented Aug 2, 2024

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nodejs-github-bot commented Aug 2, 2024

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

marco-ippolito commented Aug 2, 2024

Copy link

Copy Markdown

Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ShogunPanda approved these changes

Aug 2, 2024

View reviewed changes

jakecastelli reviewed

Aug 2, 2024

View reviewed changes

Comment thread

test/fixtures/typescript/ts/test-import-fs.ts

		@@ -0,0 +1,2 @@
		import fs from 'fs';

jakecastelli Aug 2, 2024

Copy link

Copy Markdown

Member

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.

nit: in other typescript fixtures we have node: prefix

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

himself65 reacted with thumbs up emoji

All reactions

👍 1 reaction

jakecastelli approved these changes

Aug 2, 2024

View reviewed changes

mcollina approved these changes

Aug 2, 2024

View reviewed changes

mcollina left a comment

Copy link

Copy Markdown

Member

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.

lgtm

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

himself65 approved these changes

Aug 2, 2024

View reviewed changes

marco-ippolito added author ready

PRs with CI started, the required approvals, and no outstanding review comments.

commit-queue PRs queued for automated landing through the Commit Queue. labels

Aug 2, 2024

anonrig approved these changes

Aug 3, 2024

View reviewed changes

nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label

Aug 3, 2024

nodejs-github-bot merged commit 5d6c76a into nodejs:main

Aug 3, 2024

nodejs-github-bot commented Aug 3, 2024

Copy link

Copy Markdown

Collaborator

Landed in 5d6c76a

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

marco-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-ippolito mentioned this pull request

Aug 5, 2024

[v22.x] Backport "module: fix strip-types interaction with detect-module" #54211

Closed

marco-ippolito added a commit to marco-ippolito/node that referenced this pull request

Aug 5, 2024


          module: fix strip-types interaction with detect-module

01631f3

PR-URL: nodejs#54164
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Jake Yuesong Li <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Zeyu "Alex" Yang <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>

marco-ippolito added a commit that referenced this pull request

Aug 5, 2024


          module: fix strip-types interaction with detect-module

c20e841

PR-URL: #54164
Backport-PR-URL: #54211
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Jake Yuesong Li <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Zeyu "Alex" Yang <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>

targos 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

Reviewers

aduh95

aduh95 left review comments

mcollina

mcollina approved these changes

ShogunPanda

ShogunPanda approved these changes

anonrig

anonrig approved these changes

himself65

himself65 approved these changes

jakecastelli

jakecastelli approved these changes

+2 more reviewers

targos

targos left review comments

khaosdoctor

khaosdoctor approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready

PRs with CI started, the required approvals, and no outstanding review comments.

backported-to-v22.x

PRs backported to the v22.x-staging branch.

lib / src

Issues and PRs involving general changes in the lib/ or src/ directories.

needs-ci

PRs that need a full CI run.

strip-types

Issues and PRs related to TypeScript type stripping.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

10 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

marco-ippolito commented Aug 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Aug 1, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Aug 1, 2024

Uh oh!

codecov Bot commented Aug 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nodejs-github-bot commented Aug 2, 2024

Uh oh!

nodejs-github-bot commented Aug 2, 2024

Uh oh!

nodejs-github-bot commented Aug 2, 2024

Uh oh!

nodejs-github-bot commented Aug 2, 2024

Uh oh!

marco-ippolito commented Aug 2, 2024

Uh oh!

jakecastelli Aug 2, 2024

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Aug 3, 2024

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

marco-ippolito commented Aug 1, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

codecov Bot commented Aug 1, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.