module: add specific error for dir import

[aduh95]

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.

[ljharb]

cc @nodejs/modules-active-members

[addaleax]

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

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

[guybedford]

@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]

@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]

@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]

@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]

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]

@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:

1. 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.
2. 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.
3. 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:

1. Make directory resolution errors a special new type of error (both with a trailing / and without)
2. 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]

@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]

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

[addaleax]

@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]

@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]

@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]

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

[aduh95]

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]

@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]

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

[aduh95]

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

[ljharb]

lgtm

[aduh95]

@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

[nodejs-github-bot]

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

[aduh95]

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]

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]

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

[guybedford]

Landed in 985e9c5 and 1cb80d1.