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.

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

module: add module.stripTypeScriptTypes #55282

Merged
merged 1 commit into from
Oct 24, 2024

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
doc/api/vm.md Outdated Show resolved Hide resolved
doc/api/vm.md Outdated Show resolved Hide resolved
doc/api/vm.md Outdated Show resolved Hide resolved
doc/api/vm.md Outdated Show resolved Hide resolved
doc/api/vm.md Outdated Show resolved Hide resolved
lib/internal/modules/helpers.js Outdated Show resolved Hide resolved
lib/internal/modules/helpers.js Outdated Show resolved Hide resolved
@RedYetiDev RedYetiDev 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
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 219 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

lib/vm.js Outdated Show resolved Hide resolved
lib/vm.js Outdated Show resolved Hide resolved
lib/vm.js Outdated Show resolved Hide resolved
lib/vm.js Outdated Show resolved Hide resolved
@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.

@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

@nodejs-github-bot nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Oct 24, 2024
@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

@marco-ippolito marco-ippolito added request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Oct 24, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 24, 2024
@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
$

🤞

@atlowChemi atlowChemi added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 24, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 24, 2024
@nodejs-github-bot nodejs-github-bot merged commit 53b1050 into nodejs:main Oct 24, 2024
59 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 53b1050

RafaelGSS pushed a commit that referenced this pull request Nov 1, 2024
PR-URL: #55282
Fixes: #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>
Reviewed-By: Richard Lau <rlau@redhat.com>
RafaelGSS added a commit to RafaelGSS/node that referenced this pull request Nov 1, 2024
Notable changes:

doc:
  * move typescript support to active development (Marco Ippolito) nodejs#55536
  * add jazelly to collaborators (Jason Zhang) nodejs#55531
fs:
  * (SEMVER-MINOR) make `dirent.path` writable (Antoine du Hamel) nodejs#55547
http:
  * (SEMVER-MINOR) add diagnostic channel `http.client.request.created` (Marco Ippolito) nodejs#55586
module:
  * (SEMVER-MINOR) add `findPackageJSON` util (Jacob Smith) nodejs#55412
  * (SEMVER-MINOR) add module.stripTypeScriptTypes (Marco Ippolito) nodejs#55282

PR-URL: TODO
louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
PR-URL: nodejs#55282
Fixes: nodejs#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>
Reviewed-By: Richard Lau <rlau@redhat.com>
aduh95 added a commit that referenced this pull request Nov 6, 2024
Notable changes:

crypto:
  * update root certificates to NSS 3.104 (Richard Lau) #55681
doc:
  * move typescript support to active development (Marco Ippolito) #55536
  * add jazelly to collaborators (Jason Zhang) #55531
fs:
  * (SEMVER-MINOR) make `dirent.path` writable (Antoine du Hamel) #55547
http:
  * (SEMVER-MINOR) add diagnostic channel `http.client.request.created` (Marco Ippolito) #55586
module:
  * (SEMVER-MINOR) add `findPackageJSON` util (Jacob Smith) #55412
  * (SEMVER-MINOR) add module.stripTypeScriptTypes (Marco Ippolito) #55282
tools:
  * fix root certificate updater (Richard Lau) #55681

PR-URL: TODO
aduh95 added a commit that referenced this pull request Nov 6, 2024
Notable changes:

crypto:
  * update root certificates to NSS 3.104 (Richard Lau) #55681
doc:
  * move typescript support to active development (Marco Ippolito) #55536
  * add jazelly to collaborators (Jason Zhang) #55531
fs:
  * (SEMVER-MINOR) make `dirent.path` writable (Antoine du Hamel) #55547
http:
  * (SEMVER-MINOR) add diagnostic channel `http.client.request.created` (Marco Ippolito) #55586
module:
  * (SEMVER-MINOR) add `findPackageJSON` util (Jacob Smith) #55412
  * (SEMVER-MINOR) add `module.stripTypeScriptTypes` (Marco Ippolito) #55282

PR-URL: #55741
aduh95 added a commit that referenced this pull request Nov 6, 2024
Notable changes:

crypto:
  * update root certificates to NSS 3.104 (Richard Lau) #55681
doc:
  * move typescript support to active development (Marco Ippolito) #55536
  * add jazelly to collaborators (Jason Zhang) #55531
fs:
  * (SEMVER-MINOR) make `dirent.path` writable (Antoine du Hamel) #55547
http:
  * (SEMVER-MINOR) add diagnostic channel `http.client.request.created` (Marco Ippolito) #55586
module:
  * (SEMVER-MINOR) add `findPackageJSON` util (Jacob Smith) #55412
  * (SEMVER-MINOR) add `module.stripTypeScriptTypes` (Marco Ippolito) #55282

PR-URL: #55741
aduh95 added a commit that referenced this pull request Nov 9, 2024
Notable changes:

crypto:
  * update root certificates to NSS 3.104 (Richard Lau) #55681
doc:
  * move typescript support to active development (Marco Ippolito) #55536
  * add jazelly to collaborators (Jason Zhang) #55531
fs:
  * (SEMVER-MINOR) make `dirent.path` writable (Antoine du Hamel) #55547
http:
  * (SEMVER-MINOR) add diagnostic channel `http.client.request.created` (Marco Ippolito) #55586
module:
  * (SEMVER-MINOR) add `findPackageJSON` util (Jacob Smith) #55412
  * (SEMVER-MINOR) add `module.stripTypeScriptTypes` (Marco Ippolito) #55282

PR-URL: #55741
aduh95 added a commit that referenced this pull request Nov 11, 2024
Notable changes:

crypto:
  * update root certificates to NSS 3.104 (Richard Lau) #55681
doc:
  * move typescript support to active development (Marco Ippolito) #55536
  * add jazelly to collaborators (Jason Zhang) #55531
fs:
  * (SEMVER-MINOR) make `dirent.path` writable (Antoine du Hamel) #55547
http:
  * (SEMVER-MINOR) add diagnostic channel `http.client.request.created` (Marco Ippolito) #55586
module:
  * (SEMVER-MINOR) add `findPackageJSON` util (Jacob Smith) #55412
  * (SEMVER-MINOR) add `module.stripTypeScriptTypes` (Marco Ippolito) #55282

PR-URL: #55741
aduh95 added a commit that referenced this pull request Nov 11, 2024
Notable changes:

crypto:
  * update root certificates to NSS 3.104 (Richard Lau) #55681
doc:
  * move typescript support to active development (Marco Ippolito) #55536
  * add jazelly to collaborators (Jason Zhang) #55531
fs:
  * (SEMVER-MINOR) make `dirent.path` writable (Antoine du Hamel) #55547
http:
  * (SEMVER-MINOR) add diagnostic channel `http.client.request.created` (Marco Ippolito) #55586
module:
  * (SEMVER-MINOR) add `findPackageJSON` util (Jacob Smith) #55412
  * (SEMVER-MINOR) add `module.stripTypeScriptTypes` (Marco Ippolito) #55282

PR-URL: #55741
tpoisseau pushed a commit to tpoisseau/node that referenced this pull request Nov 21, 2024
PR-URL: nodejs#55282
Fixes: nodejs#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>
Reviewed-By: Richard Lau <rlau@redhat.com>
tpoisseau pushed a commit to tpoisseau/node that referenced this pull request Nov 21, 2024
Notable changes:

crypto:
  * update root certificates to NSS 3.104 (Richard Lau) nodejs#55681
doc:
  * move typescript support to active development (Marco Ippolito) nodejs#55536
  * add jazelly to collaborators (Jason Zhang) nodejs#55531
fs:
  * (SEMVER-MINOR) make `dirent.path` writable (Antoine du Hamel) nodejs#55547
http:
  * (SEMVER-MINOR) add diagnostic channel `http.client.request.created` (Marco Ippolito) nodejs#55586
module:
  * (SEMVER-MINOR) add `findPackageJSON` util (Jacob Smith) nodejs#55412
  * (SEMVER-MINOR) add `module.stripTypeScriptTypes` (Marco Ippolito) nodejs#55282

PR-URL: nodejs#55741
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Nov 26, 2024
Notable changes:

crypto:
  * update root certificates to NSS 3.104 (Richard Lau) nodejs#55681
doc:
  * move typescript support to active development (Marco Ippolito) nodejs#55536
  * add jazelly to collaborators (Jason Zhang) nodejs#55531
fs:
  * (SEMVER-MINOR) make `dirent.path` writable (Antoine du Hamel) nodejs#55547
http:
  * (SEMVER-MINOR) add diagnostic channel `http.client.request.created` (Marco Ippolito) nodejs#55586
module:
  * (SEMVER-MINOR) add `findPackageJSON` util (Jacob Smith) nodejs#55412
  * (SEMVER-MINOR) add `module.stripTypeScriptTypes` (Marco Ippolito) nodejs#55282

PR-URL: nodejs#55741
@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
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. backport-requested-v22.x PRs awaiting manual backport 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