module: add specific error for dir import #33220

aduh95 · 2020-05-03T10:17:17Z

Fixes: #33219

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

guybedford

Did not have to wait long for this PR, thank you @aduh95. Just a few comments - let me know if you have any questions.

lib/internal/modules/esm/get_format.js

ljharb

cc @nodejs/modules-active-members

lib/internal/modules/esm/get_format.js

addaleax · 2020-05-03T21:00:41Z

So … maybe I’m missing something, but I feel like this makes the error message more correct, but not necessarily more helpful. The original situation is one like this:

package.json:

{
  "exports": {
    "import": "./bar.mjs"
  }
}

bar.mjs:

export default 42;

foo.mjs:

import './';

As a user, neither Unknown file extension nor Cannot find module really tell me

whether I can import directories by pointing to their paths, and
if I can, what the wrong thing with the above approach is.

test/parallel/test-directory-import.js

guybedford · 2020-05-03T21:32:46Z

@addaleax with the PR at #31906 landed the error message will look like:

Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/path/to/dir/' imported from ...
Did you mean to import ./dir/index.js?
    at finalizeResolution (internal/modules/esm/resolve.js:*:*)
    at moduleResolve (internal/modules/esm/resolve.js:*:*)
    at Loader.defaultResolve [as _resolve] (internal/modules/esm/resolve.js:*:*)
    at Loader.resolve (internal/modules/esm/loader.js:*:*)
    at Loader.getModuleJob (internal/modules/esm/loader.js:*:*)
    at Loader.import (internal/modules/esm/loader.js:*:*)
    at internal/process/esm_loader.js:*:*
    at Object.initializeLoader (internal/process/esm_loader.js:*:*)
    at runMainESM (internal/modules/run_main.js:*:*)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:*:*) {
  code: 'ERR_MODULE_NOT_FOUND'

Hopefully that should make this clearer I think.

addaleax · 2020-05-03T21:35:48Z

@guybedford With this PR, rebased against master, I get the following in the scenario above:

Error [ERR_MODULE_NOT_FOUND]: Cannot find module 'file:///tmp/y/' imported from [object Object]
    at Loader.defaultGetFormat [as _getFormat] (internal/modules/esm/get_format.js:68:15)

Which is about as helpful as the current message coming from Node.js 14.x.

guybedford · 2020-05-03T21:38:09Z

@addaleax right, we definitely need the hint as I've explained to be output in this PR. If you have other suggestions please feel free to provide them as well.

addaleax · 2020-05-03T21:42:42Z

@guybedford I guess my main issue is that neither Unsupported file extension nor Could not find module really make sense here, so the error message should be more specific than that. The module is there – it seems to me like importing it in this specific way is not allowed, and if that is correct, that it would be great if the error message were to state that a) this is not a “could not find” situation but a “could not use this way” situation, and b) what kind of usage would be allowed to make this work.

aduh95 · 2020-05-03T21:50:33Z

FWIW I agree with @addaleax, I think it would more helpful to have a more specific error. I feel we should also use this opportunity to promote package exports and package self-referencing for people coming from CJS.

guybedford · 2020-05-03T21:53:04Z

@addaleax thanks for explaining your thoughts here. These are good points!

If you don't mind diving in a little.... Ok... so, the current logic is:

resolve: If the resolved path is not a file it throws module not found. This means module not found is thrown for all directory resolutions, or resolution to any other descriptor which is not a file.
import.meta.resolve: If the resolved path ends with / it specifically opts out of the stat check to support the use case of await import.meta.resolve('./dir/') always working out, despite dir being a directory. This allows eg - mkdirp.sync(await import.meta.resolve('./dir/') sort of asset resolution use cases. Perhaps supporting resolving directories that don't exist isn't necessary here but treating / as a sort of opt-out was quite nice to allow these sorts of "free" path resolutions.
This PR: Specifically deals with the case of the trailing / since that is what is throwing in the resolution.

The root cause of this issue is thus really the feature added in (2).

Based on your feedback, it sounds like the suggested path forward would be to:

Make directory resolution errors a special new type of error (both with a trailing / and without)
It would then be possible to have import.meta.resolve run a resolver that recovers from this new error type specifically to allow those resolver use cases.

Note that neither (1) or (2) directly relate to this PR. But I think that would be the best breadth of solution to property tackle this based on the discussion here.

Let me know how that sounds further.

addaleax · 2020-05-03T21:59:01Z

@guybedford Not sure I’m qualified to speak to a lot here, but 1. sounds good.

Lots of people might have the expectation that, since require('./') works, import './'; should work as well, but that appears not to be the case and I think that’s what the more specific error message should clarify.

guybedford · 2020-05-03T22:01:22Z

@addaleax what do you think we should do for other file descriptors? The same error, module not found, or another one?

addaleax · 2020-05-03T22:03:09Z

@guybedford I’m not really sure that I understand where file descriptors come into play here, or if you’re using that term to mean something other than system interface file descriptors, what it is that you’re referring to?

guybedford · 2020-05-03T22:16:34Z

@addaleax just wondering, while we are refining these semantics, if isFile() and isDirectory() comprehensively cover the code path on https://github.com/nodejs/node/blob/master/lib/internal/modules/esm/resolve.js#L188 corresponding to the two errors, or if there are other types of paths like sockets that we need to think about what error they would give.

addaleax · 2020-05-03T22:20:46Z

@guybedford I think that line should read if (fstatSync(fd).isDirectory()) return undefined; instead – treat directories like directories, and anything else like regular files, like most other programs. But I don’t think a separate error is necessary for other file types.

guybedford · 2020-05-03T22:21:57Z

Ok sounds good! Let's do that then and thanks for the feedback here.

aduh95 · 2020-05-03T22:36:30Z

An issue I have is the getFormat hook doesn't have access to the original specifier used by the user, so instead I'm using the directory URL. That makes the error more verbose and is less helpful for debugging. Would it be fine to add the actual specifier to the context object?

$ out/Release/node test/parallel/test-directory-import.js
.../node/test/common/index.js:600
const crashOnUnhandledRejection = (err) => { throw err; };
                                             ^

Error [ERR_MODULE_NOT_FOUND]: Cannot find module '.../node/test/fixtures/packages/main' imported from .../node/test/parallel/test-directory-import.js
Did you mean to import .../node/test/fixtures/packages/main/package-main-module.js?
    at finalizeResolution (internal/modules/esm/resolve.js:277:11)
    at moduleResolve (internal/modules/esm/resolve.js:658:10)
    at Loader.defaultResolve [as _resolve] (internal/modules/esm/resolve.js:767:11)
    at Loader.resolve (internal/modules/esm/loader.js:97:40)
    at Loader.getModuleJob (internal/modules/esm/loader.js:243:28)
    at Loader.import (internal/modules/esm/loader.js:178:28)
    at importModuleDynamically (internal/modules/cjs/loader.js:1116:29)
    at importModuleDynamicallyWrapper (internal/vm/module.js:432:21)
    at importModuleDynamically (vm.js:376:43)
    at exports.importModuleDynamicallyCallback (internal/process/esm_loader.js:29:14) {
  code: 'ERR_MODULE_NOT_FOUND'
}

guybedford · 2020-05-03T22:40:15Z

@aduh95 let's move the directory error right back into the resolver to make this easier by removing this code path here - https://github.com/nodejs/node/blob/master/lib/internal/modules/esm/resolve.js#L273 and adding the new directory error for isDirectory() at https://github.com/nodejs/node/blob/master/lib/internal/modules/esm/resolve.js#L276. Then import.meta.resolve implementation can specifically detect the directory error and allow it in its wrapping here - https://github.com/nodejs/node/blob/master/lib/internal/modules/esm/translators.js#L61 to keep the import meta resolve tests passing. We should also update the esm resolver spec in https://github.com/nodejs/node/blob/master/doc/api/esm.md#resolver-algorithm under ESM_RESOLVE to reflect this new error code.

It's a lot I know... but let me know if that all makes sense?

guybedford · 2020-05-03T22:41:01Z

Sorry the first code line should be this one - https://github.com/nodejs/node/blob/master/lib/internal/modules/esm/resolve.js#L273.

aduh95 · 2020-05-03T22:45:53Z

@guybedford thanks for your last comment, I was about to ask for a TLDR 😅 I'll work on it tomorrow!

lib/internal/modules/esm/resolve.js

Fixes: nodejs#33219

ljharb

lgtm

doc/api/esm.md

Co-authored-by: Jordan Harband <[email protected]>

aduh95 · 2020-05-14T18:23:15Z

@guybedford Any chance you have time to review this? I have implemented the changes you requested, if you want to have another look.

guybedford

Looks good.

//cc @nodejs/modules-active-members

test/parallel/test-directory-import.js

nodejs-github-bot · 2020-05-14T18:48:20Z

CI: https://ci.nodejs.org/job/node-test-pull-request/31337/

Adds hint when module specifier is a file URL. Refs: nodejs#31906

aduh95 · 2020-05-14T23:37:56Z

There was a bug in #31906 when the module specifier is a file URL, it wouldn't print the Did you mean hint. I have added ed3dfc5 to address that. I believe it's fine to have it part of this PR but if you think it deserves its own PR, I can make that.

guybedford · 2020-05-15T19:13:52Z

Great to see, yes I think that is fine to include that in this PR! Will run CI again now to land.

nodejs-github-bot · 2020-05-15T19:14:43Z

CI: https://ci.nodejs.org/job/node-test-pull-request/31353/

PR-URL: #33220 Fixes: #33219 Reviewed-By: Guy Bedford <[email protected]>

Adds hint when module specifier is a file URL. PR-URL: #33220 Fixes: #33219 Reviewed-By: Guy Bedford <[email protected]>

guybedford · 2020-05-16T02:09:47Z

Landed in 985e9c5 and 1cb80d1.

PR-URL: #33220 Fixes: #33219 Reviewed-By: Guy Bedford <[email protected]>

Adds hint when module specifier is a file URL. PR-URL: #33220 Fixes: #33219 Reviewed-By: Guy Bedford <[email protected]>

PR-URL: #33220 Fixes: #33219 Reviewed-By: Guy Bedford <[email protected]>

Adds hint when module specifier is a file URL. PR-URL: #33220 Fixes: #33219 Reviewed-By: Guy Bedford <[email protected]>

PR-URL: nodejs#33220 Fixes: nodejs#33219 Reviewed-By: Guy Bedford <[email protected]>

Adds hint when module specifier is a file URL. PR-URL: nodejs#33220 Fixes: nodejs#33219 Reviewed-By: Guy Bedford <[email protected]>

nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. esm Issues and PRs related to the ECMAScript Modules implementation. labels May 3, 2020

guybedford reviewed May 3, 2020

View reviewed changes

lib/internal/modules/esm/get_format.js Outdated Show resolved Hide resolved

lib/internal/modules/esm/get_format.js Outdated Show resolved Hide resolved

ljharb reviewed May 3, 2020

View reviewed changes

lib/internal/modules/esm/get_format.js Outdated Show resolved Hide resolved

guybedford reviewed May 3, 2020

View reviewed changes

test/parallel/test-directory-import.js Outdated Show resolved Hide resolved

aduh95 force-pushed the improve-import-dir-error branch from b629609 to 0ef0588 Compare May 3, 2020 22:32

aduh95 marked this pull request as draft May 3, 2020 22:46

aduh95 force-pushed the improve-import-dir-error branch 2 times, most recently from 4bb6e0b to 80a6427 Compare May 4, 2020 10:51

aduh95 commented May 4, 2020

View reviewed changes

lib/internal/modules/esm/resolve.js Outdated Show resolved Hide resolved

aduh95 marked this pull request as ready for review May 4, 2020 17:36

ljharb reviewed May 4, 2020

View reviewed changes

lib/internal/modules/esm/resolve.js Outdated Show resolved Hide resolved

module: add specific error for dir import

11cb078

Fixes: nodejs#33219

fixup

e3da38d

aduh95 requested review from ljharb and guybedford May 5, 2020 10:21

ljharb reviewed May 5, 2020

View reviewed changes

doc/api/esm.md Outdated Show resolved Hide resolved

fixup: nit

1471091

Co-authored-by: Jordan Harband <[email protected]>

guybedford approved these changes May 14, 2020

View reviewed changes

guybedford reviewed May 14, 2020

View reviewed changes

test/parallel/test-directory-import.js Outdated Show resolved Hide resolved

fixup: Windows tests

112c552

esm: improve commonjs hint on module not found

ed3dfc5

Adds hint when module specifier is a file URL. Refs: nodejs#31906

guybedford pushed a commit that referenced this pull request May 16, 2020

module: add specific error for dir import

985e9c5

PR-URL: #33220 Fixes: #33219 Reviewed-By: Guy Bedford <[email protected]>

guybedford pushed a commit that referenced this pull request May 16, 2020

esm: improve commonjs hint on module not found

1cb80d1

Adds hint when module specifier is a file URL. PR-URL: #33220 Fixes: #33219 Reviewed-By: Guy Bedford <[email protected]>

guybedford closed this May 16, 2020

aduh95 mentioned this pull request May 16, 2020

doc: add package.json documentation #32970

Closed

3 tasks

codebytere pushed a commit that referenced this pull request May 16, 2020

module: add specific error for dir import

35aae31

PR-URL: #33220 Fixes: #33219 Reviewed-By: Guy Bedford <[email protected]>

codebytere pushed a commit that referenced this pull request May 16, 2020

esm: improve commonjs hint on module not found

e53de96

Adds hint when module specifier is a file URL. PR-URL: #33220 Fixes: #33219 Reviewed-By: Guy Bedford <[email protected]>

codebytere mentioned this pull request May 18, 2020

v14.3.0 proposal #33452

Merged

codebytere pushed a commit that referenced this pull request Jun 7, 2020

module: add specific error for dir import

9cc7b24

PR-URL: #33220 Fixes: #33219 Reviewed-By: Guy Bedford <[email protected]>

codebytere pushed a commit that referenced this pull request Jun 7, 2020

esm: improve commonjs hint on module not found

1c238b6

Adds hint when module specifier is a file URL. PR-URL: #33220 Fixes: #33219 Reviewed-By: Guy Bedford <[email protected]>

codebytere pushed a commit to codebytere/node that referenced this pull request Jun 9, 2020

module: add specific error for dir import

4f6e4ae

PR-URL: nodejs#33220 Fixes: nodejs#33219 Reviewed-By: Guy Bedford <[email protected]>

codebytere pushed a commit to codebytere/node that referenced this pull request Jun 9, 2020

esm: improve commonjs hint on module not found

a029dca

Adds hint when module specifier is a file URL. PR-URL: nodejs#33220 Fixes: nodejs#33219 Reviewed-By: Guy Bedford <[email protected]>

codebytere mentioned this pull request Jun 9, 2020

v12.18.1 proposal #33811

Merged

aduh95 deleted the improve-import-dir-error branch November 14, 2020 11:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

module: add specific error for dir import #33220

module: add specific error for dir import #33220

aduh95 commented May 3, 2020

guybedford left a comment

ljharb left a comment

addaleax commented May 3, 2020

guybedford commented May 3, 2020

addaleax commented May 3, 2020

guybedford commented May 3, 2020

addaleax commented May 3, 2020

aduh95 commented May 3, 2020

guybedford commented May 3, 2020

addaleax commented May 3, 2020

guybedford commented May 3, 2020

addaleax commented May 3, 2020

guybedford commented May 3, 2020 •

edited

Loading

addaleax commented May 3, 2020

guybedford commented May 3, 2020

aduh95 commented May 3, 2020

guybedford commented May 3, 2020 •

edited

Loading

guybedford commented May 3, 2020

aduh95 commented May 3, 2020

ljharb left a comment

aduh95 commented May 14, 2020

guybedford left a comment

nodejs-github-bot commented May 14, 2020

aduh95 commented May 14, 2020

guybedford commented May 15, 2020

nodejs-github-bot commented May 15, 2020

guybedford commented May 16, 2020

module: add specific error for dir import #33220

module: add specific error for dir import #33220

Conversation

aduh95 commented May 3, 2020

Checklist

guybedford left a comment

Choose a reason for hiding this comment

ljharb left a comment

Choose a reason for hiding this comment

addaleax commented May 3, 2020

guybedford commented May 3, 2020

addaleax commented May 3, 2020

guybedford commented May 3, 2020

addaleax commented May 3, 2020

aduh95 commented May 3, 2020

guybedford commented May 3, 2020

addaleax commented May 3, 2020

guybedford commented May 3, 2020

addaleax commented May 3, 2020

guybedford commented May 3, 2020 • edited Loading

addaleax commented May 3, 2020

guybedford commented May 3, 2020

aduh95 commented May 3, 2020

guybedford commented May 3, 2020 • edited Loading

guybedford commented May 3, 2020

aduh95 commented May 3, 2020

ljharb left a comment

Choose a reason for hiding this comment

aduh95 commented May 14, 2020

guybedford left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented May 14, 2020

aduh95 commented May 14, 2020

guybedford commented May 15, 2020

nodejs-github-bot commented May 15, 2020

guybedford commented May 16, 2020

guybedford commented May 3, 2020 •

edited

Loading

guybedford commented May 3, 2020 •

edited

Loading