-
-
Notifications
You must be signed in to change notification settings - Fork 34.4k
lib: fix TypeScript support check in jitless mode #61382
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
base: main
Are you sure you want to change the base?
Conversation
202f642 to
e56e36b
Compare
doc/api/errors.md
Outdated
|
|
||
| ### `ERR_WEBASSEMBLY_NOT_SUPPORTED` | ||
|
|
||
| WebAssembly is not supported. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error message does not really add much. We should specify that the internal requires webassembly to be enabled, and therefore cannot be used in environments where its not present such as when running with --jitless
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is not performing any check, it just added an error that is not used anywhere. The error should be thrown when lazily initializing the typescript parser
Thanks for the feedback. I'll review the logic and move the check to the appropriate place as suggested. I will also improve the error message to be more descriptive. |
08c7bc7 to
e6cb215
Compare
|
|
||
| The WASI instance has not been started. | ||
|
|
||
| <a id="ERR_WEBASSEMBLY_NOT_SUPPORTED"></a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the name ERR_WEBASSEMBLY_NOT_SUPPORTED incorrect, the issue is that web assembly is required and not present
lib/internal/errors.js
Outdated
| 'Provided module is not an instance of Module', Error); | ||
| E('ERR_VM_MODULE_STATUS', 'Module status %s', Error); | ||
| E('ERR_WASI_ALREADY_STARTED', 'WASI instance has already started', Error); | ||
| E('ERR_WEBASSEMBLY_NOT_SUPPORTED', 'WebAssembly is not supported', Error); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the error message should be improve, we should also pass which feature is trying to need it (like typescript)
lib/internal/modules/typescript.js
Outdated
| */ | ||
| const loadTypeScriptParser = getLazy(() => { | ||
| assertTypeScript(); | ||
| if (WebAssembly === undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should probably go in assertTypeScript
bf1ec67 to
ca857df
Compare
WebAssembly is disabled when Node.js is run with --jitless. The internal TypeScript stripper relies on WebAssembly, and previously failed obscurely with a ReferenceError in this mode. This commit adds an explicit check for WebAssembly support when loading the TypeScript parser. If WebAssembly is unavailable, it now throws a descriptive ERR_WEBASSEMBLY_NOT_SUPPORTED error. Fixes: nodejs#61353
ca857df to
ef5444a
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #61382 +/- ##
==========================================
- Coverage 88.54% 88.53% -0.01%
==========================================
Files 704 704
Lines 208793 208800 +7
Branches 40307 40312 +5
==========================================
- Hits 184866 184854 -12
- Misses 15913 15924 +11
- Partials 8014 8022 +8
🚀 New features to boost your workflow:
|
The internal TypeScript stripper relies on WebAssembly, which is disabled in --jitless mode. This PR updates
assertTypeScript to explicitly throw ERR_WEBASSEMBLY_NOT_SUPPORTED instead of failing obscurely when WebAssembly
is unavailable.
A regression test has been added to test/es-module/test-typescript.mjs.
Fixes: #61353