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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

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 2 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 RedYetiDev added the experimental Issues and PRs related to experimental features. label Sep 24, 2024
@marco-ippolito
Copy link
Member

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

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

@jasnell
Copy link
Member

jasnell commented Sep 28, 2024

CI is good but unfortunately this needs a rebase and yet another CI run.

@anonrig
Copy link
Member

anonrig commented Sep 28, 2024

I left a comment to one of my comments, that has been "resolved" but it isn't.

@RedYetiDev RedYetiDev added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Sep 28, 2024
@marco-ippolito marco-ippolito added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 1, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 1, 2024
@nodejs-github-bot
Copy link
Collaborator

@RedYetiDev RedYetiDev added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 1, 2024
@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

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. 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