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

module: add --experimental-strip-input-types #56273

Conversation

marco-ippolito
Copy link
Member

@marco-ippolito marco-ippolito commented Dec 16, 2024

Refs: nodejs/typescript#17
We cannot unflag --experimental-strip-types as it is because when using --eval it will ALWAYS emit an experimental warning and treat the input as typescript regardless.
This flag specifically enables the input of --eval to be treated as typescript that otherwise will be treated as javascript.
Once --experimental-strip-types is stable we can unflag this too.
I tested this PR with --experimental-strip-types unflagged and it goes smoothly.

This is a breaking change (it's experimental so it should not matter too much but still) since now --experimental-strip-types is no longer sufficient for input 🤷🏼‍♂️ but I don't see another way.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/typescript

@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 Dec 16, 2024
@joyeecheung
Copy link
Member

I wonder why do we have to emit the warning - can't we just strip it always for --eval and compare the stripped code with the original and see if it's modified before emitting the warning? Maybe minus the platform where swc crashes...or use something that doesn't crash for it.

@joyeecheung
Copy link
Member

Also, alternatively we can just skip the entrypoint warning for --eval, since the warning is temporary and will hopefully go away eventually, it seems simplicity of not introducing another flag that will stay after the temporary warning runs its course outweighs whatever the warning serves.

@marco-ippolito
Copy link
Member Author

marco-ippolito commented Dec 16, 2024

Even if we dont emit a warning the syntax errors are wrapped by swc.
For example function foo(){ await Promise.resolve(1)} when strip types is enabled it will throw an "INVALID_TYPESCRIPT_SYNTAX_ERROR" rather than the usual syntax error.
Also the crash is only for loaders so in eval should be fine (? I guess)

@joyeecheung
Copy link
Member

For example function foo(){ await Promise.resolve(1)} when strip types is enabled it will throw an "INVALID_TYPESCRIPT_SYNTAX_ERROR" rather than the usual syntax error.

Is that thrown during type stripping? If the error is identifyable I think we can just catch it in that case, and move on to parse/evaluate the original source?

@marco-ippolito
Copy link
Member Author

For example function foo(){ await Promise.resolve(1)} when strip types is enabled it will throw an "INVALID_TYPESCRIPT_SYNTAX_ERROR" rather than the usual syntax error.

Is that thrown during type stripping? If the error is identifyable I think we can just catch it in that case, and move on to parse/evaluate the original source?

Yes the error is thrown during type stripping.
There is no way to distinguish between a invalid typescript syntax error (example you are using enums in type stripping) vs a generic syntax error. @kdy1 maybe knows more

@marco-ippolito marco-ippolito force-pushed the unflag-experimental-strip-types branch from 588e2a7 to c5704d8 Compare December 16, 2024 17:12
@marco-ippolito
Copy link
Member Author

marco-ippolito commented Dec 16, 2024

Or a solution could be to wrap all INVALID_TYPESCRIPT_SYNTAX_ERROR when using eval in a normal syntax error 🤷🏼
The error message will change but at least should throw the same error and remove the warning.
Doing so will change the error message:

marcoippolito@marcos-MBP node % ./node --experimental-strip-types -e "function foo(){ await Promise.resolve(1) }"
node:internal/main/eval_string:41
      throw new SyntaxError(error.message);
      ^

SyntaxError:   x await isn't allowed in non-async function
   ,----
 1 | function foo(){ await Promise.resolve(1) }
   :                       ^^^^^^^
   `----


Caused by:
    failed to parse
    at node:internal/main/eval_string:41:13

Node.js v24.0.0-pre
marcoippolito@marcos-MBP node % ./node -e "function foo(){ await Promise.resolve(1) }" 
[eval]:1
function foo(){ await Promise.resolve(1) }
                ^^^^^

SyntaxError: await is only valid in async functions and the top level bodies of modules
    at makeContextifyScript (node:internal/vm:185:14)
    at node:internal/process/execution:107:22
    at [eval]-wrapper:6:24
    at runScript (node:internal/process/execution:101:62)
    at evalScript (node:internal/process/execution:136:3)
    at node:internal/main/eval_string:69:3

Node.js v24.0.0-pre

Comment on lines +998 to +1008
### `--experimental-strip-input-types`

<!-- YAML
added: REPLACEME
-->

> Stability: 1.1 - Active development

Enable experimental type-stripping for `--eval`.
Implies [`--experimental-strip-types`][].
For more information, see the [TypeScript type-stripping][] documentation.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally this could just be a new value for --input-type, so like --input-type=strip-types; could that work? And I guess it would rely on syntax detection to determine format, or it would just always assume ESM since TypeScript syntax seems to be always ESM?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah but then dont see how it can ever be enabled by default.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could take the same approach as syntax detection: try to run as regular --eval, if it throws based on an exception that might’ve been caused by TypeScript syntax, retry as --input-type=strip-types.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes thats what Im experimenting

Copy link

codecov bot commented Dec 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.54%. Comparing base (a50f3d5) to head (c5704d8).
Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #56273      +/-   ##
==========================================
- Coverage   88.55%   88.54%   -0.01%     
==========================================
  Files         657      657              
  Lines      190243   190243              
  Branches    36536    36541       +5     
==========================================
- Hits       168461   168450      -11     
- Misses      14963    14968       +5     
- Partials     6819     6825       +6     
Files with missing lines Coverage Δ
lib/internal/main/eval_string.js 79.66% <100.00%> (ø)
lib/internal/modules/cjs/loader.js 98.11% <ø> (-0.01%) ⬇️
lib/internal/modules/esm/translators.js 92.88% <ø> (-0.05%) ⬇️
lib/internal/modules/typescript.js 100.00% <100.00%> (ø)
src/node_options.cc 88.01% <100.00%> (+0.03%) ⬆️
src/node_options.h 98.30% <100.00%> (+<0.01%) ⬆️

... and 24 files with indirect coverage changes

@marco-ippolito marco-ippolito added the wip Issues and PRs that are still a work in progress. label Dec 16, 2024
@marco-ippolito
Copy link
Member Author

Found a workaround 😄 I'll open a new PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants