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.

Sign up for GitHub

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

Jump to bottom

module: simplify ts under node_modules check #55440

Merged
merged 1 commit into from
Oct 20, 2024
Merged

module: simplify ts under node_modules check #55440

merged 1 commit into from
Oct 20, 2024

Conversation

marco-ippolito
Copy link
Member

@marco-ippolito marco-ippolito commented Oct 18, 2024 •
edited
Loading

Moves the check from random places in loaders to stripTypeScriptTypes

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders

@nodejs-github-bot nodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run. labels Oct 18, 2024
@aduh95
Copy link
Contributor

aduh95 commented Oct 18, 2024

Doesn't that mean we could not expose the utility to users so they can strip those types manually?

@marco-ippolito
Copy link
Member Author

marco-ippolito commented Oct 18, 2024 •
edited
Loading

Doesn't that mean we could not expose the utility to users so they can strip those types manually?

Not sure what you mean.
You mean we could expose the utility?

@codecov Codecov
Copy link

codecov bot commented Oct 18, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.39%. Comparing base (7ae193d) to head (775e13a).
Report is 355 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #55440      +/-   ##
==========================================
- Coverage   88.40%   88.39%   -0.02%     
==========================================
  Files         653      653              
  Lines      187600   187585      -15     
  Branches    36117    36117              
==========================================
- Hits       165854   165817      -37     
- Misses      14974    14989      +15     
- Partials     6772     6779       +7
Files with missing lines Coverage Δ
lib/internal/modules/cjs/loader.js 97.15% <ø> (-0.29%) ⬇️
lib/internal/modules/esm/load.js 91.98% <ø> (-0.27%) ⬇️
lib/internal/modules/helpers.js 99.20% <100.00%> (+0.20%) ⬆️

... and 34 files with indirect coverage changes

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

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

@nodejs-github-bot
Copy link
Collaborator

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

@marco-ippolito marco-ippolito added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 18, 2024
mcollina
mcollina approved these changes Oct 18, 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 the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 19, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 20, 2024
@nodejs-github-bot nodejs-github-bot merged commit b0ffe9e into nodejs:main Oct 20, 2024
76 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in b0ffe9e

aduh95 pushed a commit that referenced this pull request Oct 23, 2024
@marco-ippolito @aduh95
module: simplify ts under node_modules check
a12dbf0 
PR-URL: #55440
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@aduh95 aduh95 mentioned this pull request Oct 24, 2024
Merged
louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
@marco-ippolito @louwers
module: simplify ts under node_modules check
835c683 
PR-URL: nodejs#55440
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
tpoisseau pushed a commit to tpoisseau/node that referenced this pull request Nov 21, 2024
@marco-ippolito @tpoisseau
module: simplify ts under node_modules check
f4b9fec 
PR-URL: nodejs#55440
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@ruyadorno
Copy link
Member

This commit does not land cleanly on v22.x-staging and will need manual backport in case we want it in v22.x.

@ruyadorno ruyadorno added the backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. label Nov 27, 2024
marco-ippolito added a commit to marco-ippolito/node that referenced this pull request Dec 10, 2024
@marco-ippolito
module: simplify ts under node_modules check
028e6c0 
PR-URL: nodejs#55440
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@marco-ippolito marco-ippolito added the backport-open-v22.x Indicate that the PR has an open backport label Dec 10, 2024
marco-ippolito added a commit to marco-ippolito/node that referenced this pull request Dec 11, 2024
@marco-ippolito
module: simplify ts under node_modules check
706e299 
PR-URL: nodejs#55440
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
marco-ippolito added a commit to marco-ippolito/node that referenced this pull request Dec 13, 2024
@marco-ippolito
module: simplify ts under node_modules check
7f33374 
PR-URL: nodejs#55440
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
marco-ippolito added a commit to marco-ippolito/node that referenced this pull request Dec 14, 2024
@marco-ippolito
module: simplify ts under node_modules check
8aa18d1 
PR-URL: nodejs#55440
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
marco-ippolito added a commit to marco-ippolito/node that referenced this pull request Dec 18, 2024
@marco-ippolito
module: simplify ts under node_modules check
cca3af8 
PR-URL: nodejs#55440
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
marco-ippolito added a commit to marco-ippolito/node that referenced this pull request Dec 18, 2024
@marco-ippolito
module: simplify ts under node_modules check
ec6a002 
PR-URL: nodejs#55440
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
ruyadorno pushed a commit that referenced this pull request Dec 20, 2024
@marco-ippolito @ruyadorno
module: simplify ts under node_modules check
420d599 
PR-URL: #55440
Backport-PR-URL: #56208
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@ruyadorno ruyadorno mentioned this pull request Dec 20, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trivikr trivikr trivikr approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@legendecas legendecas legendecas approved these changes

@anonrig anonrig anonrig approved these changes

@benjamingr benjamingr benjamingr approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-open-v22.x Indicate that the PR has an open backport backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. esm Issues and PRs related to the ECMAScript Modules implementation. module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@marco-ippolito @nodejs-github-bot @aduh95 @ruyadorno @mcollina @benjamingr @anonrig @legendecas @BridgeAR @trivikr