process: add `process.features.typescript`

[avivkeller]

Fixes #54294

[nodejs-github-bot]

Review requested:

• [ ] @nodejs/startup

[avivkeller]

Drafting, as there may be a way to get the arg in CPP

[codecov[bot]]

Codecov Report

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

Project coverage is 88.40%. Comparing base (d24c731) to head (74a8769). Report is 74 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #54295   +/-   ##
=======================================
  Coverage   88.39%   88.40%           
=======================================
  Files         652      652           
  Lines      186568   186591   +23     
  Branches    36047    36050    +3     
=======================================
+ Hits       164924   164952   +28     
+ Misses      14915    14912    -3     
+ Partials     6729     6727    -2     

Files with missing lines	Coverage Δ
lib/internal/bootstrap/node.js	99.78% <100.00%> (+0.01%)	:arrow_up:

... and 25 files with indirect coverage changes

[H4ad]

Maybe process.features.stripTypes is not better?

Also, there's no other way to check if any flag is enabled? I know there is inside the codebase but I don't remember to see it exposed

[avivkeller]

Also, there's no other way to check if any flag is enabled? I know there is inside the codebase but I don't remember to see it exposed

It's pretty late where I am, but I'm going to investigate alternative approaches tomorrow

[avivkeller]

Maybe process.features.stripTypes is not better?

I feel like if more typescript support is added, E.G. #54283, it does more than just strip types.

[marco-ippolito]

since we have 2 levels of typescript support, instead of boolean it would be better to return { mode: "transform" // or strip-only }

[silverwind]

since we have 2 levels of typescript support, instead of boolean it would be better to return { mode: "transform" // or strip-only }

Hmm, not sure that would fit in process.features. In my case, I'm primarly after knowing whether the current process can load typescript, I wouldn't care about those modes as long as they both give typescript loading support.

[legendecas]

/cc @nodejs/typescript

[silverwind]

I don't think we should add this. This feature can easily be added as a userland module. I don't think we should add more attributes to global variables unless we have a compelling reason.

Please demonstrate how one can do this in a future-proof way that continues to work once the flag is made the default. I see none.

[marco-ippolito]

I'm ok with this feature since otherwise user need to check process.argv or process.execArgv or NODE_OPTIONS. We already have it for some other features so why not.

marcoippolito@marcos-MacBook-Pro amaro % node -p "process.features"
{
  inspector: true,
  debug: false,
  uv: true,
  ipv6: true,
  tls_alpn: true,
  tls_sni: true,
  tls_ocsp: true,
  tls: true,
  cached_builtins: [Getter]
}

But it needs to be a getter like cached_builtins and return an object

[avivkeller]

But it needs to be a getter like cached_builtins and return an object

I can update it to be a getter and return an object

[avivkeller]

Right now I have it like @legendecas suggested, that is, strip when --experimental-strip-types and transform when --experimental-transform-types, would you prefer your suggestion:

{ mode: "transform" // or strip-only }

[silverwind]

But it needs to be a getter like cached_builtins and return an object

Fine with me actually. I see now that the distinction between strip and transform mode is important, so if user does not care about those, they can get their boolean via Boolean(process.features.typescript) which will work if it returns false when not supporting any.

[richardlau]

Documentation is missing

process.features as a whole is undocumented. See https://github.com/nodejs/node/issues/25343 (and https://github.com/nodejs/node/issues/22585) for history.

[marco-ippolito]

I guess its time to document it then 😆 it has been around for a while

[marco-ippolito]

@anonrig are you still blocking?

[nodejs-github-bot]

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

[avivkeller]

~~@anonrig are you still blocking?~~ :sweat_smile: @marco-ippolito already asked.

[anonrig]

Yes. I'm still blocking. I don't think we should land this now. This is adding an attribute to a global object about an experimental feature that can clearly be implemented in the userland.

PS: this function should also emit an experimental warning.

[marco-ippolito]

Yes. I'm still blocking. I don't think we should land this now. This is adding an attribute to a global object about an experimental feature that can clearly be implemented in the userland.

PS: this function should also emit an experimental warning.

I really dont see what is the downside of adding typescript to process.features. A 3rd party library has to go through process.argv and process.execArgv and NODE_OPTIONS and when unflagged or a new flag is added (I dont think it will tbh) its a breaking change. Having it in core has better DX, better performance, minimal impact. Also the process.features is meant for this purpose afaik. I strongly feel this should land, not sure about the warning since having the flag will already emit the warning.

[jasnell]

@nodejs/tsc for visibility ... not yet putting on the agenda but would be good to have tsc folks weigh in.

[marco-ippolito]

It needs to be documented – and documented as experimental.

Documentation is missing

process.features as a whole is undocumented. See #25343 (and #22585) for history.

As mentioned by @richardlau , process.features is not documented at all.

[avivkeller]

@aduh95 When this lands, I assume #54897 will be updated, or vise versa. Are you still blocking for other reasons?

[marco-ippolito]

https://github.com/nodejs/node/pull/54897 landed

[avivkeller]

https://github.com/nodejs/node/pull/54897 landed

Lovely! I'll update the PR accordingly today.

[avivkeller]

@anonrig @aduh95 are you still blocking? If so -> Can this be added to the tsc agenda?

[aduh95]

Can this be added to the tsc agenda?

What would be the question for the TSC?

[avivkeller]

What would be the question for the TSC?

I meant to override the previously blocking reviews.

[marco-ippolito]

@anonrig can you check if your concerns have been resolved?