Uh oh!
There was an error while loading. Please reload this page.
modules: enforce extensions and dissalow directories - #21572
Conversation
this build on top of the limitations of the package name map proposal. It enforces the use of file extensions and doesn't support index.js or package.json resolution for the default ESM loader implementation. The current approach is naive and has lots of room for improvement but this can serve as a base for discussion Refs: https://github.com/domenic/package-name-maps
| An attempt was made to use features that require [ICU][], but Node.js was not | ||
| compiled with ICU support. | ||
| <a id="ERR_NON_EXPLICIT_SPECIFIER"></a> |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Yes, sorry, N is before _. But then it should go before ERR_NO_CRYPTO, right?
| <a id="ERR_NON_EXPLICIT_SPECIFIER"></a> | ||
| ### ERR_NON_EXPLICIT_SPECIFIER | ||
| Modules that are imported must use explicit file extensions |
| ### Mandatory file extensions | ||
| You must provide a file extension when using the `import` keyword. |
There was a problem hiding this comment.
We usually avoid you in API docs, as per style guide.
| E('ERR_NAPI_INVALID_TYPEDARRAY_LENGTH', | ||
| 'Invalid typed array length', RangeError); | ||
| E('ERR_NON_EXPLICIT_SPECIFIER', | ||
| 'The file %s does not include an extensionimported modules must declare' + |
There was a problem hiding this comment.
extensionimported -> extension. Imported?
| throw new ERR_MISSING_MODULE(target); | ||
| } | ||
| if ((target[0] === '.' || target[0] === '/') && | ||
| (extname(target) <= 1)) { |
There was a problem hiding this comment.
It may be a little clearer to have an explicit .length check here instead of relying on JavaScript magic.
| // We cannot search without a base. | ||
| throw new ERR_MISSING_MODULE(target); | ||
| } | ||
| if ((target[0] === '.' || target[0] === '/') && |
There was a problem hiding this comment.
This check should be target[0] === '.' && (target[1] === '/' || target[1] === '.' && target[2] === '/') || target[0] === '/' to ensure support for filenames starting with .. Alternatively just the regex /^\.{0,2}\//.test(target).
I understand this is an initial implementation, but note that this implementation still supports "main"s and URLs getting default extensions. Happy to do a deeper review in due course.
There was a problem hiding this comment.
maybe String.prototype.startsWith can be used?
There was a problem hiding this comment.
If we do use a method, i'd prefer the non-taintable approaches used elsewhere for robustness
bmeck
commented
Jun 28, 2018
We should discuss migration strategies here in depth and the impact of this, in particular this seems at odds with the findings of nodejs/modules#139 about migration strategies that support a smooth ability to migrate. It would be good if the overall reasoning about why matching package-name-maps is seen as superior to the migration path findings in that issue. |
bmeck
left a comment
There was a problem hiding this comment.
this should not land without heavy discussion and documentation of a migration strategy existing somewhere
MylesBorins
commented
Jul 9, 2018
Closing in lieu of #21729 |
this build on top of the limitations of the package name map proposal. It enforces the use of file extensions and doesn't support index.js or package.json resolution for the default ESM loader implementation.
The current approach is naive and has lots of room for improvement but this can serve as a base for discussion
Refs: https://github.com/domenic/package-name-maps