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
nodejs-github-bot
added
the
errors
| @@ -1388,6 +1388,11 @@ OpenSSL crypto support. | |||
| 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.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Yes, sorry, N is before _. But then it should go before ERR_NO_CRYPTO, right?
doc/api/errors.md
Outdated
| <a id="ERR_NON_EXPLICIT_SPECIFIER"></a> | ||
| ### ERR_NON_EXPLICIT_SPECIFIER | ||
|
|
||
| Modules that are imported must use explicit file extensions |
doc/api/esm.md
Outdated
| @@ -54,6 +54,14 @@ property: | |||
|
|
|||
| ## Notable differences between `import` and `require` | |||
|
|
|||
| ### 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.
lib/internal/errors.js
Outdated
| @@ -740,6 +740,9 @@ E('ERR_NAPI_INVALID_TYPEDARRAY_ALIGNMENT', | |||
| 'start offset of %s should be a multiple of %s', RangeError); | |||
| 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?
MylesBorins
added
esm
| @@ -24,6 +25,10 @@ function search(target, base) { | |||
| // We cannot search without a base. | |||
| 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.
| @@ -24,6 +25,10 @@ function search(target, base) { | |||
| // 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
|
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. |
|
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