-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
changes the base instance for ERR_UNKNOWN_FILE_EXTENSION #20069
Conversation
from Error to TypeError as a more accurate representation of the error and adds a unit test for unsupported extension in esm resolution.
type: TypeError, | ||
message: 'Unknown file extension: ' + unsupported | ||
} | ||
); |
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.
Would you be so kind and post an example how a user could actually run into such an error without using any internals directly?
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.
import foo from 'file://path/to/module.unsupported'
To be clear though: this isn't a JavaScript Type-mismatch error, right? Are you sure this is appropriate? |
@Fishrock123 see discussions on #19805 |
also, to a lesser extent: #19807 |
I think in this case it is probably better to keep it as is. So I would just remove the comment and keep the test case. |
This needs at least one more TSC approval. |
To me it still seems that it would be best to keep the error as is. |
Only commenting because TSC is being specifically asked for reviews, so just to explain why I'm not approving nor objecting to this: I don't object, but I also don't endorse the changes to Likely TSC approvers might be @mcollina @joyeecheung @TimothyGu Meta: Should there be a @nodejs/errors team? |
@BridgeAR If you're expressing an opinion but not blocking, cool. I suspect that's what's going on. However, in case I'm wrong: If you mean to be objecting but perhaps are trying to be gentle about it, I'd recommend using Request Changes anyway. It doesn't sound like you're objecting. |
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.
Failures in CI on Windows machines appear to be relevant. So much for not reviewing this change. :-D
not ok 514 es-module/test-esm-loader
---
duration_ms: 0.159
severity: fail
exitcode: 1
stack: |-
assert.js:246
throw err;
^
AssertionError [ERR_ASSERTION]: Error [ERR_MODULE_RESOLUTION_LEGACY] is not instance of TypeError
at Object.innerFn (c:\workspace\node-test-binary-windows\test\common\index.js:702:7)
at expectedException (assert.js:428:19)
at expectsError (assert.js:511:16)
at Function.throws (assert.js:542:3)
at Object.expectsError (c:\workspace\node-test-binary-windows\test\common\index.js:736:12)
at Object.<anonymous> (c:\workspace\node-test-binary-windows\test\es-module\test-esm-loader.js:18:8)
at Module._compile (internal/modules/cjs/loader.js:678:30)
at Object.Module._extensions..js (internal/modules/cjs/loader.js:689:10)
at Module.load (internal/modules/cjs/loader.js:589:32)
at tryModuleLoad (internal/modules/cjs/loader.js:528:12)
...
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.
Making it explicit.
Ping @davidmarkclements |
Closing since there was no response for a long time and explicit -1. @davidmarkclements if you would like to continue working on this / feel like this should be worked on again, please feel free to leave a comment or to open a new PR. |
from Error to TypeError as a more accurate representation
of the error and adds a unit test for unsupported extension
in esm resolution.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes