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

process: add process.features.typescript #54295

Merged
merged 2 commits into from
Oct 5, 2024

Conversation

RedYetiDev
Copy link
Member

Fixes #54294

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Aug 10, 2024
@RedYetiDev RedYetiDev marked this pull request as draft August 10, 2024 00:15
@RedYetiDev
Copy link
Member Author

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

Copy link

codecov bot commented Aug 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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%) ⬆️

... and 25 files with indirect coverage changes

@H4ad
Copy link
Member

H4ad commented Aug 10, 2024

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

@RedYetiDev
Copy link
Member Author

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

@RedYetiDev
Copy link
Member Author

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.

@RedYetiDev RedYetiDev added process Issues and PRs related to the process subsystem. strip-types Issues or PRs related to strip-types support labels Aug 10, 2024
@RedYetiDev RedYetiDev marked this pull request as ready for review August 10, 2024 16:07
@marco-ippolito
Copy link
Member

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

@silverwind
Copy link
Contributor

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
Copy link
Member

/cc @nodejs/typescript

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.

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.

@silverwind
Copy link
Contributor

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
Copy link
Member

marco-ippolito commented Aug 20, 2024

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

@RedYetiDev
Copy link
Member Author

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

@RedYetiDev
Copy link
Member Author

RedYetiDev commented Aug 20, 2024

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 }

@RedYetiDev
Copy link
Member Author

Do I need to rebase to resolve the github CI failures, or can this land without them?

@richardlau
Copy link
Member

Do I need to rebase to resolve the github CI failures, or can this land without them?

Unfortunately, yes, you will need to rebase.

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

@RedYetiDev
Copy link
Member Author

This needs yet another rebase 😭

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

targos pushed a commit that referenced this pull request Oct 4, 2024
PR-URL: #54897
Refs: #54295
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@nodejs-github-bot
Copy link
Collaborator

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

Landed in bbdfeeb

targos pushed a commit that referenced this pull request Oct 5, 2024
PR-URL: #54295
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@aduh95 aduh95 mentioned this pull request Oct 9, 2024
@aduh95 aduh95 added the semver-minor PRs that contain new features and should be released in the next minor version. label Oct 12, 2024
aduh95 added a commit that referenced this pull request Oct 13, 2024
Notable changes:

crypto:
  * (SEMVER-MINOR) add `KeyObject.prototype.toCryptoKey` (Filip Skokan) #55262
  * (SEMVER-MINOR) add Date fields for `validTo` and `validFrom` (Andrew Moon) #54159
doc:
  * add abmusse to collaborators (Abdirahim Musse) #55086
http2:
  * (SEMVER-MINOR) expose `nghttp2_option_set_stream_reset_rate_limit` as an option (Maël Nison) #54875
lib:
  * (SEMVER-MINOR) propagate aborted state to dependent signals before firing events (jazelly) #54826
module:
  * (SEMVER-MINOR) support loading entrypoint as url (RedYetiDev) #54933
  * (SEMVER-MINOR) implement the `"module-sync"` exports condition (Joyee Cheung) #54648
  * (SEMVER-MINOR) implement `flushCompileCache()` (Joyee Cheung) #54971
  * (SEMVER-MINOR) throw when invalid argument is passed to `enableCompileCache()` (Joyee Cheung) #54971
  * (SEMVER-MINOR) write compile cache to temporary file and then rename it (Joyee Cheung) #54971
process:
  * (SEMVER-MINOR) add `process.features.require_module` (Joyee Cheung) #55241
  * (SEMVER-MINOR) add `process.features.typescript` (Aviv Keller) #54295
src:
  * mark `node --run` as stable (Yagiz Nizipli) #53763
test_runner:
  * (SEMVER-MINOR) support custom arguments in `run()` (Aviv Keller) #55126
  * (SEMVER-MINOR) add `'test:summary'` event (Colin Ihrig) #54851
  * (SEMVER-MINOR) add support for coverage via `run()` (Chemi Atlow) #53937
worker:
  * (SEMVER-MINOR) add `markAsUncloneable` api (Jason Zhang) #55234

PR-URL: #55343
aduh95 added a commit that referenced this pull request Oct 13, 2024
Notable changes:

crypto:
  * (SEMVER-MINOR) add `KeyObject.prototype.toCryptoKey` (Filip Skokan) #55262
  * (SEMVER-MINOR) add Date fields for `validTo` and `validFrom` (Andrew Moon) #54159
doc:
  * add abmusse to collaborators (Abdirahim Musse) #55086
http2:
  * (SEMVER-MINOR) expose `nghttp2_option_set_stream_reset_rate_limit` as an option (Maël Nison) #54875
lib:
  * (SEMVER-MINOR) propagate aborted state to dependent signals before firing events (jazelly) #54826
module:
  * (SEMVER-MINOR) support loading entrypoint as url (RedYetiDev) #54933
  * (SEMVER-MINOR) implement the `"module-sync"` exports condition (Joyee Cheung) #54648
  * (SEMVER-MINOR) implement `flushCompileCache()` (Joyee Cheung) #54971
  * (SEMVER-MINOR) throw when invalid argument is passed to `enableCompileCache()` (Joyee Cheung) #54971
  * (SEMVER-MINOR) write compile cache to temporary file and then rename it (Joyee Cheung) #54971
process:
  * (SEMVER-MINOR) add `process.features.require_module` (Joyee Cheung) #55241
  * (SEMVER-MINOR) add `process.features.typescript` (Aviv Keller) #54295
src:
  * mark `node --run` as stable (Yagiz Nizipli) #53763
test_runner:
  * (SEMVER-MINOR) support custom arguments in `run()` (Aviv Keller) #55126
  * (SEMVER-MINOR) add `'test:summary'` event (Colin Ihrig) #54851
  * (SEMVER-MINOR) add support for coverage via `run()` (Chemi Atlow) #53937
worker:
  * (SEMVER-MINOR) add `markAsUncloneable` api (Jason Zhang) #55234

PR-URL: #55343
aduh95 added a commit that referenced this pull request Oct 16, 2024
Notable changes:

crypto:
  * (SEMVER-MINOR) add `KeyObject.prototype.toCryptoKey` (Filip Skokan) #55262
  * (SEMVER-MINOR) add Date fields for `validTo` and `validFrom` (Andrew Moon) #54159
doc:
  * add abmusse to collaborators (Abdirahim Musse) #55086
http2:
  * (SEMVER-MINOR) expose `nghttp2_option_set_stream_reset_rate_limit` as an option (Maël Nison) #54875
lib:
  * (SEMVER-MINOR) propagate aborted state to dependent signals before firing events (jazelly) #54826
module:
  * (SEMVER-MINOR) support loading entrypoint as url (RedYetiDev) #54933
  * (SEMVER-MINOR) implement the `"module-sync"` exports condition (Joyee Cheung) #54648
  * (SEMVER-MINOR) implement `flushCompileCache()` (Joyee Cheung) #54971
  * (SEMVER-MINOR) throw when invalid argument is passed to `enableCompileCache()` (Joyee Cheung) #54971
  * (SEMVER-MINOR) write compile cache to temporary file and then rename it (Joyee Cheung) #54971
process:
  * (SEMVER-MINOR) add `process.features.require_module` (Joyee Cheung) #55241
  * (SEMVER-MINOR) add `process.features.typescript` (Aviv Keller) #54295
src:
  * mark `node --run` as stable (Yagiz Nizipli) #53763
test_runner:
  * (SEMVER-MINOR) support custom arguments in `run()` (Aviv Keller) #55126
  * (SEMVER-MINOR) add `'test:summary'` event (Colin Ihrig) #54851
  * (SEMVER-MINOR) add support for coverage via `run()` (Chemi Atlow) #53937
worker:
  * (SEMVER-MINOR) add `markAsUncloneable` api (Jason Zhang) #55234

PR-URL: #55343
aduh95 added a commit that referenced this pull request Oct 16, 2024
Notable changes:

crypto:
  * (SEMVER-MINOR) add `KeyObject.prototype.toCryptoKey` (Filip Skokan) #55262
  * (SEMVER-MINOR) add Date fields for `validTo` and `validFrom` (Andrew Moon) #54159
doc:
  * add abmusse to collaborators (Abdirahim Musse) #55086
http2:
  * (SEMVER-MINOR) expose `nghttp2_option_set_stream_reset_rate_limit` as an option (Maël Nison) #54875
lib:
  * (SEMVER-MINOR) propagate aborted state to dependent signals before firing events (jazelly) #54826
module:
  * (SEMVER-MINOR) support loading entrypoint as url (RedYetiDev) #54933
  * (SEMVER-MINOR) implement the `"module-sync"` exports condition (Joyee Cheung) #54648
  * (SEMVER-MINOR) implement `flushCompileCache()` (Joyee Cheung) #54971
  * (SEMVER-MINOR) throw when invalid argument is passed to `enableCompileCache()` (Joyee Cheung) #54971
  * (SEMVER-MINOR) write compile cache to temporary file and then rename it (Joyee Cheung) #54971
process:
  * (SEMVER-MINOR) add `process.features.require_module` (Joyee Cheung) #55241
  * (SEMVER-MINOR) add `process.features.typescript` (Aviv Keller) #54295
src:
  * mark `node --run` as stable (Yagiz Nizipli) #53763
test_runner:
  * (SEMVER-MINOR) support custom arguments in `run()` (Aviv Keller) #55126
  * (SEMVER-MINOR) add `'test:summary'` event (Colin Ihrig) #54851
  * (SEMVER-MINOR) add support for coverage via `run()` (Chemi Atlow) #53937
worker:
  * (SEMVER-MINOR) add `markAsUncloneable` api (Jason Zhang) #55234

PR-URL: #55343
louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
PR-URL: nodejs#54897
Refs: nodejs#54295
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
PR-URL: nodejs#54295
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
Notable changes:

crypto:
  * (SEMVER-MINOR) add `KeyObject.prototype.toCryptoKey` (Filip Skokan) nodejs#55262
  * (SEMVER-MINOR) add Date fields for `validTo` and `validFrom` (Andrew Moon) nodejs#54159
doc:
  * add abmusse to collaborators (Abdirahim Musse) nodejs#55086
http2:
  * (SEMVER-MINOR) expose `nghttp2_option_set_stream_reset_rate_limit` as an option (Maël Nison) nodejs#54875
lib:
  * (SEMVER-MINOR) propagate aborted state to dependent signals before firing events (jazelly) nodejs#54826
module:
  * (SEMVER-MINOR) support loading entrypoint as url (RedYetiDev) nodejs#54933
  * (SEMVER-MINOR) implement the `"module-sync"` exports condition (Joyee Cheung) nodejs#54648
  * (SEMVER-MINOR) implement `flushCompileCache()` (Joyee Cheung) nodejs#54971
  * (SEMVER-MINOR) throw when invalid argument is passed to `enableCompileCache()` (Joyee Cheung) nodejs#54971
  * (SEMVER-MINOR) write compile cache to temporary file and then rename it (Joyee Cheung) nodejs#54971
process:
  * (SEMVER-MINOR) add `process.features.require_module` (Joyee Cheung) nodejs#55241
  * (SEMVER-MINOR) add `process.features.typescript` (Aviv Keller) nodejs#54295
src:
  * mark `node --run` as stable (Yagiz Nizipli) nodejs#53763
test_runner:
  * (SEMVER-MINOR) support custom arguments in `run()` (Aviv Keller) nodejs#55126
  * (SEMVER-MINOR) add `'test:summary'` event (Colin Ihrig) nodejs#54851
  * (SEMVER-MINOR) add support for coverage via `run()` (Chemi Atlow) nodejs#53937
worker:
  * (SEMVER-MINOR) add `markAsUncloneable` api (Jason Zhang) nodejs#55234

PR-URL: nodejs#55343
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. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. experimental Issues and PRs related to experimental features. needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. 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.

Indicate typescript support in process.features