lib: expose tsParse as util.stripTypescriptTypes

[stagas]

This PR exposes internal/modules/helpers/tsParse as util.stripTypescriptTypes in order to transpile/strip types from TypeScript code without bringing in another dependency, since it's already there.

All details are in flux, I just picked the quickest solution I could come up with, but I wanted to bring it here for discussion.

Primary use-case would be transpiling modules to serve to the client from a pure node server without bringing in dependencies.

[nodejs-github-bot]

Review requested:

• @nodejs/startup
• @nodejs/typescript

[anonrig]

I don't think we should land this at it's current state, or since it is based on an experimental feature.

1. The function shouldnt live under process and exposed globally. Ideally, it should live in node:util
2. tsParse function name is not DX friendly. parseTypescript() or stripTypescriptTypes() might be a better naming.
3. I think we should wait until strip types experimental feature becomes stable.

[stagas]

@anonrig I agree with 1 and 2 but I think this should be included in the experimental feature. It is simply so useful and it's already there. Even at stable we should still have a way to transpile without a dependency, so this isn't going away. We simply need to expose it at the right location. I can modify the PR according to those suggestions.

[aduh95]

We should also document it, and expose it only when --experimental-strip-types is passed

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.11%. Comparing base (e0634f5) to head (19d3296).
Report is 4 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #54250   +/-   ##
=======================================
  Coverage   87.10%   87.11%           
=======================================
  Files         647      647           
  Lines      181739   181740    +1     
  Branches    34887    34882    -5     
=======================================
+ Hits       158310   158316    +6     
  Misses      16738    16738           
+ Partials     6691     6686    -5     

Files	Coverage Δ
lib/internal/bootstrap/node.js	99.77% <100.00%> (+<0.01%)	⬆️

... and 27 files with indirect coverage changes

[legendecas]

IMO a user should explicit depend on an amaro version if they want to use it programmatically. The --experimental-strip-types flag should only provide stability guarantee where the .ts files are transpiled.

The API is another topic of stability guarantee. An independently installed amaro dependnecy can reduce the chance the API is break at this stage.

/cc @nodejs/typescript

[marco-ippolito]

I'm -1 on exposing the internal of an experimental feature that its gonna change.
You can install amaro and use the transformSync function that its exactly the one that its used in core https://github.com/nodejs/amaro
Agree that tsParse is horrible and changing it would be great, PR welcome 😄

[stagas]

@anonrig @aduh95 trying to use getOptionValue under node:util brings an error that it hasn't been bootstrapped yet and can't query for options. So it has to go after bootstrap or some other solution is needed. Need help here.

@legendecas if we are to bring a dependency to do it then that defeats the purpose of having no dependency programatic transpile in node. This is a useful feature to have in core IMO. So many projects need this and now they pick whatever dependency to transpile and it becomes an incompatible mess, it would be cool if all were using the same and tools were built on top of that.

[stagas]

@marco-ippolito but it's part of the same feature, it's the mechanism of stripping TS types, in module importing and manually. TS is here to stay, projects are going to need to strip the types in the backend, why make everyone bring in their dependency when we already have one and we're going to have one. The details don't matter, whether it's amaro or something else in the future, the goal is we want to use TS and strip the TS types to serve to the client. This is the feature.

[marco-ippolito]

@marco-ippolito but it's part of the same feature, it's the mechanism of stripping TS types, in module importing and manually. TS is here to stay, projects are going to need to strip the types in the backend, why make everyone bring in their dependency when we already have one and we're going to have one. The details don't matter, whether it's amaro or something else in the future, the goal is we want to use TS and strip the TS types to serve to the client. This is the feature.

In the past we have made the mistake of exposing internals and users have started to rely on them, making it very hard to iterate. Actually if its behind the --experiemental-strip-types it would be enough.
This PR is still missing documentation. Also can rename tsParse to something more reasonable.

[benjamingr]

Needs docs and more tests but otherwise lgtm (expose behind the same flag)

[khaosdoctor]

I also agree on the point that we shouldn't expose internal APIs, in my view it's a good thing to have everyone using less deps and relying more on Node directly, but this should only be the case when Node actually starts supporting TS out of the box, otherwise, at least for me it gets a bit confusing.

I'm not against having it behind the flag though, it would be useful eventually, but still, to me it looks like something that's dangling, like it doesn't have an actual home.

[avivkeller]

If #54287 lands, when a non-string is passed, it'll throw an assertion. I don't think this function, if exposed to users, should throw an internal assertion.

changed my mind

[marco-ippolito]

This needs a rethink after the change of #54283

[sosoba]

This needs a rethink after the change of #54283

util.transformTypescriptTypes ?

[jdalton]

This needs a rethink after the change of #54283

util.transformTypescriptTypes ?

minor nit: TS => TypeScript not Typescript