module: fix specifier resolution algorithm #30574
Conversation
|
Can you add tests? |
|
@cjihrig OK, I'll try to add some tests for this case. |
|
I know I suggested this approach as a simple way to resolve the issue, but I don't think we should support The alternative would be to use the ESM resolver when this flag is set. That can be achieved by having the |
|
And yes, tests very much necessary too. |
|
Hi @guybedford, I've updated the implementation and tests for this case. |
| @@ -24,6 +24,10 @@ function shouldUseESMLoader(mainPath) { | |||
| const userLoader = getOptionValue('--experimental-loader'); | |||
| if (userLoader) | |||
| return true; | |||
| const esModuleSpecifierResolution = | |||
| getOptionValue('--es-module-specifier-resolution'); | |||
There was a problem hiding this comment.
Perfect. One thing to note is that this flag is experimental. I just posted an issue about this here - #30600.
Since this is now the primary entry for this flag, we should probably move this check up to the top of the function, and also include a warning like:
process.emitWarning(
'The --es-module-specifier-resolution flag is experimental.',
'ExperimentalWarning', undefined);Would help to add this stuff while we're working on this area anyway, but if you'd rather it be done separately that is fine too just thought I'd mention as it's important for users not to overlook that any of these flags might be removed in future.
There was a problem hiding this comment.
Thanks for the explanation. I've noticed that #30600 is not only about specifier-resolution but also related to other experimental flags, so perhaps the fix about it should be raised in another PR rather than merged to this one.
There was a problem hiding this comment.
If I don't understand it wrong, I'll try to work on #30600 and create another PR to fix it.
There was a problem hiding this comment.
Sure if you're able to work on that it would be great, just mentioning as otherwise it can be useful to do these things while we're there.
MylesBorins
added
the
esm
Trott
added
the
author ready
|
Landed in f5ef7cd |
Fixes: nodejs#30520 PR-URL: nodejs#30574 Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Myles Borins <[email protected]>
Fixes: #30520 PR-URL: #30574 Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Myles Borins <[email protected]>
Fixes: #30520 PR-URL: #30574 Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Myles Borins <[email protected]>
Fixes: #30520 PR-URL: #30574 Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Myles Borins <[email protected]>
Fixes: #30520
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes