Skip to content

module: add module.stripTypeScriptTypes#55282

Merged
nodejs-github-bot merged 1 commit intonodejs:mainfrom
marco-ippolito:vm-strip-types
Oct 24, 2024
Merged

module: add module.stripTypeScriptTypes#55282
nodejs-github-bot merged 1 commit intonodejs:mainfrom
marco-ippolito:vm-strip-types

Conversation

@marco-ippolito
Copy link
Member

@marco-ippolito marco-ippolito commented Oct 5, 2024

This PR introduces a new api in node:module.

Fixes: #54300

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 5, 2024

Review requested:

  • @nodejs/loaders
  • @nodejs/vm

@nodejs-github-bot nodejs-github-bot added module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run. vm Issues and PRs related to the vm subsystem. labels Oct 5, 2024
@marco-ippolito marco-ippolito added the strip-types Issues or PRs related to strip-types support label Oct 5, 2024
@avivkeller avivkeller added the semver-minor PRs that contain new features and should be released in the next minor version. label Oct 5, 2024
@joyeecheung
Copy link
Member

joyeecheung commented Oct 5, 2024

I think it makes more sense for it to be just in its own API as a function, which can be used to strip the type before the text is passed into any of the vm.Script/vm.SourceTextModule/vm.compileFunction APIs. That'll also be useful for those who want to transform the text but not compile it at all. The transform option being ignored without the flag could be somewhat surprising, especially paired with cachedData which only has a length check, so it could lead to crashes when V8 tries to reparse the untransformed source for error reporting.

@marco-ippolito marco-ippolito changed the title vm: add support for --experimental-strip-types vm: add vm.stripTypeScriptTypes(code, options) Oct 6, 2024
@devsnek
Copy link
Member

devsnek commented Oct 6, 2024

there's also #54250

@marco-ippolito marco-ippolito force-pushed the vm-strip-types branch 2 times, most recently from eca9ab2 to f41d422 Compare October 6, 2024 07:46
@marco-ippolito
Copy link
Member Author

marco-ippolito commented Oct 6, 2024

there's also #54250

Unfortunately it seems to have been stalled for a while.
Anyways good to review, I'm just not sure whether to be strict and throw when mode is strip-only and sourceMap is passed, otherwise will be ignored and could be counter-intuitive

@marco-ippolito marco-ippolito marked this pull request as ready for review October 6, 2024 07:49
@codecov
Copy link

codecov bot commented Oct 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.42%. Comparing base (7b5d660) to head (8532f1e).
Report is 1390 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #55282   +/-   ##
=======================================
  Coverage   88.41%   88.42%           
=======================================
  Files         653      654    +1     
  Lines      187476   187552   +76     
  Branches    36083    36087    +4     
=======================================
+ Hits       165763   165839   +76     
- Misses      14946    14951    +5     
+ Partials     6767     6762    -5     
Files with missing lines Coverage Δ
lib/internal/main/eval_string.js 80.00% <100.00%> (ø)
lib/internal/modules/cjs/loader.js 97.64% <100.00%> (+0.05%) ⬆️
lib/internal/modules/esm/get_format.js 89.32% <100.00%> (+0.04%) ⬆️
lib/internal/modules/esm/translators.js 93.09% <100.00%> (ø)
lib/internal/modules/helpers.js 98.84% <100.00%> (-0.17%) ⬇️
lib/internal/modules/typescript.js 100.00% <100.00%> (ø)
lib/module.js 100.00% <100.00%> (ø)

... and 28 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@marco-ippolito marco-ippolito force-pushed the vm-strip-types branch 2 times, most recently from 50afe56 to 9ab643f Compare October 7, 2024 07:27
@legendecas
Copy link
Member

I think this could be benefited from a dedicated module parser (name bikeshed welcome), instead of vm since the updated PR doesn't involve with any code evaluation. This module could provide additional helpers to address similar needs on manipulating JS source codes.

@joyeecheung
Copy link
Member

Maybe putting it on module also makes sense? Or module.typescript if we can foresee adding other typescript-related utilities in the future

@marco-ippolito
Copy link
Member Author

marco-ippolito commented Oct 7, 2024

I think this could be benefited from a dedicated module parser (name bikeshed welcome), instead of vm since the updated PR doesn't involve with any code evaluation. This module could provide additional helpers to address similar needs on manipulating JS source codes.

I'm ok with a new module, I think parser is ok, I'd like to hear more opinions about it
I wouldnt call it typescript and leave it more generic, maybe someone wants to add ast manipulation or bundling etc... could be the right place

@marco-ippolito marco-ippolito changed the title vm: add vm.stripTypeScriptTypes(code, options) lib: add parser.stripTypeScriptTypes Oct 7, 2024
@marco-ippolito
Copy link
Member Author

marco-ippolito commented Oct 7, 2024

If we decide to ship it I'll drop the vm commits
Pinging TSC as adding a new module seems like a big deal @nodejs/tsc

@joyeecheung
Copy link
Member

joyeecheung commented Oct 7, 2024

If this needs to be a new module, then it should be added to

const schemelessBlockList = new SafeSet([
to block require('parser') without the node: prefix, otherwise it breaks https://www.npmjs.com/package/parser or anyone creating an alias of parser in node_modules.

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

@nodejs-github-bot
Copy link
Collaborator

@mhdawson
Copy link
Member

Looks like this is moving forward, removing TSC agenda lable after discussion in todays meeting, please re-add if there is still something the TSC needs to discuss

@marco-ippolito
Copy link
Member Author

Looks like this is moving forward, removing TSC agenda lable after discussion in todays meeting, please re-add if there is still something the TSC needs to discuss

its still blocked by @anonrig review

Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

It was an error on my side. I didn't get the previous notifications and assumed that we're still creating a new module.

@marco-ippolito marco-ippolito added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Oct 24, 2024
@avivkeller avivkeller removed tsc-agenda Issues and PRs to discuss during the meetings of the TSC. review wanted PRs that need reviews. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Oct 24, 2024
@avivkeller
Copy link
Member

avivkeller commented Oct 24, 2024

Hey, I've read over the past few messages (and the entire issue), and based on them, I've adjusted the labels of this issue, LMK if that was a mistake, as this was on the agenda for TSC, but per the comments above, I've removed it.

edit: I think we simultaneously edited the labels 😅

@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/55282
✔  Done loading data for nodejs/node/pull/55282
----------------------------------- PR info ------------------------------------
Title      module: add module.stripTypeScriptTypes (#55282)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     marco-ippolito:vm-strip-types -> nodejs:main
Labels     semver-minor, tsc-agenda, needs-ci, review wanted, commit-queue-squash, strip-types
Commits    1
 - module: add module.stripTypeScriptTypes
Committers 1
 - Marco Ippolito <marcoippolito54@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/55282
Fixes: https://github.com/nodejs/node/issues/54300
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/55282
Fixes: https://github.com/nodejs/node/issues/54300
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sat, 05 Oct 2024 16:28:53 GMT
   ✔  Approvals: 6
   ✔  - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/55282#pullrequestreview-2391013128
   ✔  - Joyee Cheung (@joyeecheung) (TSC): https://github.com/nodejs/node/pull/55282#pullrequestreview-2350567333
   ✔  - Chemi Atlow (@atlowChemi): https://github.com/nodejs/node/pull/55282#pullrequestreview-2354092903
   ✔  - Paolo Insogna (@ShogunPanda) (TSC): https://github.com/nodejs/node/pull/55282#pullrequestreview-2365865817
   ✔  - Chengzhong Wu (@legendecas) (TSC): https://github.com/nodejs/node/pull/55282#pullrequestreview-2380232467
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/55282#pullrequestreview-2380688805
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-10-21T17:43:56Z: https://ci.nodejs.org/job/node-test-pull-request/63238/
- Querying data for job/node-test-pull-request/63238/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 55282
fatal: couldn't find remote ref refs/pull/55282/merge
https://github.com/nodejs/node/actions/runs/11491778677

Copy link
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

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

LGTM!

@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/55282
✔  Done loading data for nodejs/node/pull/55282
----------------------------------- PR info ------------------------------------
Title      module: add module.stripTypeScriptTypes (#55282)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     marco-ippolito:vm-strip-types -> nodejs:main
Labels     semver-minor, needs-ci, commit-queue-squash, strip-types
Commits    1
 - module: add module.stripTypeScriptTypes
Committers 1
 - Marco Ippolito <marcoippolito54@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/55282
Fixes: https://github.com/nodejs/node/issues/54300
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/55282
Fixes: https://github.com/nodejs/node/issues/54300
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sat, 05 Oct 2024 16:28:53 GMT
   ✔  Approvals: 6
   ✔  - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/55282#pullrequestreview-2391013128
   ✔  - Joyee Cheung (@joyeecheung) (TSC): https://github.com/nodejs/node/pull/55282#pullrequestreview-2350567333
   ✔  - Chemi Atlow (@atlowChemi): https://github.com/nodejs/node/pull/55282#pullrequestreview-2354092903
   ✔  - Paolo Insogna (@ShogunPanda) (TSC): https://github.com/nodejs/node/pull/55282#pullrequestreview-2391920279
   ✔  - Chengzhong Wu (@legendecas) (TSC): https://github.com/nodejs/node/pull/55282#pullrequestreview-2380232467
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/55282#pullrequestreview-2380688805
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-10-24T02:58:22Z: https://ci.nodejs.org/job/node-test-pull-request/63238/
- Querying data for job/node-test-pull-request/63238/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 55282
fatal: couldn't find remote ref refs/pull/55282/merge
https://github.com/nodejs/node/actions/runs/11496802024

@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/55282
✔  Done loading data for nodejs/node/pull/55282
----------------------------------- PR info ------------------------------------
Title      module: add module.stripTypeScriptTypes (#55282)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     marco-ippolito:vm-strip-types -> nodejs:main
Labels     semver-minor, author ready, needs-ci, strip-types
Commits    1
 - module: add module.stripTypeScriptTypes
Committers 1
 - Marco Ippolito <marcoippolito54@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/55282
Fixes: https://github.com/nodejs/node/issues/54300
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/55282
Fixes: https://github.com/nodejs/node/issues/54300
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sat, 05 Oct 2024 16:28:53 GMT
   ✔  Approvals: 6
   ✔  - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/55282#pullrequestreview-2391013128
   ✔  - Joyee Cheung (@joyeecheung) (TSC): https://github.com/nodejs/node/pull/55282#pullrequestreview-2350567333
   ✔  - Chemi Atlow (@atlowChemi): https://github.com/nodejs/node/pull/55282#pullrequestreview-2354092903
   ✔  - Paolo Insogna (@ShogunPanda) (TSC): https://github.com/nodejs/node/pull/55282#pullrequestreview-2391920279
   ✔  - Chengzhong Wu (@legendecas) (TSC): https://github.com/nodejs/node/pull/55282#pullrequestreview-2380232467
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/55282#pullrequestreview-2380688805
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-10-24T09:47:58Z: https://ci.nodejs.org/job/node-test-pull-request/63238/
- Querying data for job/node-test-pull-request/63238/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 55282
fatal: couldn't find remote ref refs/pull/55282/merge
https://github.com/nodejs/node/actions/runs/11498595438

@marco-ippolito
Copy link
Member Author

marco-ippolito commented Oct 24, 2024

Downloading patch for 55282fatal: couldn't find remote ref refs/pull/55282/merge
same error when trying manually with git node land

Let me try rebase and see if it makes difference

@nodejs-github-bot
Copy link
Collaborator

@richardlau
Copy link
Member

Downloading patch for 55282fatal: couldn't find remote ref refs/pull/55282/merge same error when trying manually with git node land

Let me try rebase and see if it makes difference

FWIW Prior to the rebase I was seeing:

$ git ls-remote upstream refs/pull/55282/*
c1c262a134f21bb350e0dfe4c8c50e2009195a04        refs/pull/55282/head
$

which was consistent with the error (no refs/pull/55282/merge). After the rebase:

$ git ls-remote upstream refs/pull/55282/*
8532f1e1e07538901a102c1880700f767d0e6686        refs/pull/55282/head
efb62871253f691abd0ac770e7361f20af13d1d2        refs/pull/55282/merge
$

🤞

@nodejs-github-bot
Copy link
Collaborator

Landed in 53b1050

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version. strip-types Issues or PRs related to strip-types support

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support types in vm.compileFunction