module: add module.stripTypeScriptTypes

[marco-ippolito]

This PR introduces a new api in node:module.

Fixes: https://github.com/nodejs/node/issues/54300

[nodejs-github-bot]

Review requested:

• [ ] @nodejs/loaders
• [ ] @nodejs/vm

[joyeecheung]

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.

[devsnek]

there's also #54250

[marco-ippolito]

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

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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%)	:arrow_up:
lib/internal/modules/esm/get_format.js	89.32% <100.00%> (+0.04%)	:arrow_up:
lib/internal/modules/esm/translators.js	93.09% <100.00%> (ø)
lib/internal/modules/helpers.js	98.84% <100.00%> (-0.17%)	:arrow_down:
lib/internal/modules/typescript.js	100.00% <100.00%> (ø)
lib/module.js	100.00% <100.00%> (ø)

... and 28 files with indirect coverage changes

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[legendecas]

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]

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]

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]

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]

If this needs to be a new module, then it should be added to https://github.com/nodejs/node/blob/b4e8f1b6bb3616ba222c4513218aa1fa9d543d8e/lib/internal/bootstrap/realm.js#L130 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.

[devsnek]

i think if the goal is to expose a generic ts api, then something from npm should be used. if the goal is to expose "handle ts modules the way the current node process does", it should be exported from node:module.

[marco-ippolito]

Done, I also refactored the typescript internals in its own file.

[legendecas]

I believe the API exposed was not limited to "handle ts modules the way the current node process does": it allows transform mode even if the process is in strip mode or, without ts-support enabled. So IMO this lies more in "generic" API.

[marco-ippolito]

I dont have a strong opinion on whether should go on a new module or in the node:module one. Maybe create a new module when we have more than 1 api? If its experimental it can be moved without going through a deprecation cycle.

[legendecas]

To be honest, I don't feel node:module is a good place for such utilities as the function don't involve with the module loading / affecting the module system, like in cases like vm.compileFunction.

[marco-ippolito]

I see there is some disagreement about where this feature should go. After playing around a bit I think creating a new module is the right thing. The reason is:

• It should not live in vm since it doesnt run code
• It should not live in module since its not really related to a particular module system. Its unrelated to --experimental-strip-types since it does not depend on the flag

[richardlau]

• It should not live in module since its not really related to a particular module system. Its unrelated to --experimental-strip-types since it does not depend on the flag

It does, however, depend on amaro being available (not the case when configured --without-amaro).

[marco-ippolito]

It does, however, depend on amaro being available (not the case when configured --without-amaro).

Made sure an error is thrown when the module or features that require typescript are used but compiled without amaro

[anonrig]

I see there is some disagreement about where this feature should go.

After playing around a bit I think creating a new module is the right thing.

The reason is:

• It should not live in vm since it doesnt run code

• It should not live in module since its not really related to a particular module system. Its unrelated to --experimental-strip-types since it does not depend on the flag

@marco-ippolito do you see a different parser added into this new module soon? I don't think we should add a new module just for a single function. Why don't we put it under util?

[marco-ippolito]

@anonrig I think there is the possibility of adding more functions to manipulate code. But imho this new feature does not belong in any of the current modules

[legendecas]

Certainly node:util can be a nest for any functions. Requiring the node: prefix, a new core module would help categorize new functionality more precisely without shadowing ecosystem packages.

I think the scope of the new function is clear that it provides new functionality of parsing and manipulating JS source codes, without evaluating them.

[anonrig]

@marco-ippolito We have parseEnv method in node:util. So this means we will add that function to node:parser as well?

[marco-ippolito]

@marco-ippolito We have parseEnv file in node:util. So this means we will add that function to node:parser as well?

Yes, it manipulates source code without evaluating it (even though its not js)

[github-actions[bot]]

Failed to start CI

   ⚠  Something was pushed to the Pull Request branch since the last approving review.
   ✘  Refusing to run CI on potentially unsafe PR

https://github.com/nodejs/node/actions/runs/11327838941

[marco-ippolito]

@anonrig I feel like I motivated the decision in previous comment, that function that manipulate source code without evaluating should be moved to this module, like parseEnv, not the opposite. Everything can go in util. Pinging @nodejs/tsc as I dont agree this should go in util.

[mcollina]

@marco-ippolito I think this is a textbook case for calling a vote.

I dislike the use of name node:parser, something like node:code_transformations would fit better or even as an utility in node:module.

util is fine, as long as the dependency is loaded dynamically.

[marco-ippolito]

I moved it to the node:module module (lol) I moved all the typescript related stuff in a separate file because it was bloating the module/helpers file. Hope this reduces the disagreements around this feature without going to a vote. cc @anonrig

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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