esm: better error message using commonjs hint #31906

dnlup · 2020-02-21T20:29:04Z

This PR tries to implement the suggestions made in issues:

When a module is not found the error message also reports the path of the module that the CommonJS resolver would have found, if any.

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

himself65

Error: Cannot find module /home/travis/build/nodejs/node/test/common/fixures imported from /home/travis/build/nodejs/node/
    at Loader.defaultResolve [as _resolve] (internal/modules/esm/resolve.js:71:11)
    at Loader.resolve (internal/modules/esm/loader.js:85:40)
    at Loader.getModuleJob (internal/modules/esm/loader.js:191:28)
    at Loader.import (internal/modules/esm/loader.js:166:28)
    at internal/process/esm_loader.js:69:31
    at Object.initializeLoader (internal/process/esm_loader.js:73:5)
    at runMainESM (internal/modules/run_main.js:45:20)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:68:5)
    at internal/main/run_main_module.js:17:47 {
  code: 'ERR_MODULE_NOT_FOUND'
}

himself65 · 2020-02-22T13:14:03Z

node --experimental-loader /no/exist.mjs index.js

it will appear

Error: Cannot find module /no/exist.js imported from xxx/xxx/index.js

I think we can't directly use commjs module loader

dnlup · 2020-02-22T14:18:11Z

Thanks @himself65 , there was a typo in the module path I was requiring. I can't reproduce the error message you posted in your last comment.

test/message/esm_loader_not_found_cjs_hint.out

guybedford

This is looking great - thanks @dnlup for your work and apologies for the delayed review.

I'd love to get this merged in soon if you can take a look at the feedback.

lib/internal/modules/esm/resolve.js

dnlup · 2020-03-21T15:51:44Z

@guybedford Sorry for the late come back. I tried to implement the suggestions you made. I had problems finding a way to test the error message on a bare specifier. I think I would have to add a node_modules folder that would be resolved from the cwd of the running test. Am I wrong? Do you have some suggestions on how to do this without putting a node_modules folder in the root of the project?

PS: I did test it manually though

lib/internal/modules/esm/resolve.js

devsnek · 2020-03-22T06:55:50Z

if it helps, we used to have this: https://github.com/nodejs/node/blob/v11.x/lib/internal/modules/esm/default_resolve.js#L39.

dnlup · 2020-04-15T05:44:58Z

I added two tests to check the behavior suggested by @guybedford and rebased against master.

dnlup · 2020-04-15T18:49:37Z

Tests are failing because I am getting a different output on stdout (this line is different). I'll investigate 🙏

guybedford · 2020-04-15T19:44:26Z

Yes it seems like it's just an extra line needed there now.

This is looking really nice, that the suggestion is exactly what the user should type is great!

Let me know when this is ready for review.

MylesBorins

This is awesome work and LGTM.

This is going to be a huge help for lots of folks, thanks for putting in the time to get this working.

lib/internal/modules/esm/resolve.js

nodejs-github-bot · 2020-04-17T18:00:50Z

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

lib/internal/modules/esm/resolve.js

dnlup · 2020-04-18T08:10:46Z

Thanks, everyone for the review and the feedback. I am trying to fix the errors on Windows then this should be ready. Sorry if it's taking a long time.

Run CommonJS resolver only if `error.code` is ERR_MODULE_NOT_FOUND. Avoid using absolute paths in hint by: * using a relative (to the parent) path if the specifier is a relative path * using a `pkg/x.js` format if the specifier is bare (e.g. `pkg/x`)

dnlup · 2020-04-21T08:07:54Z

I think we are good to go, could you take another look?

guybedford · 2020-05-02T21:35:41Z

Just running CI, then lets get this merged asap.

nodejs-github-bot · 2020-05-02T21:35:43Z

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

Run CommonJS resolver only if `error.code` is ERR_MODULE_NOT_FOUND. Avoid using absolute paths in hint by: * using a parent-relative path if the specifier is a relative path * using a `pkg/x.js` format if the specifier is bare (e.g. `pkg/x`) PR-URL: #31906 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>

guybedford · 2020-05-03T18:30:00Z

Landed in 8f87d25. Thanks so much @dnlup for your work on this!!

dnlup · 2020-05-03T18:50:48Z

Thank you all for your help!

Run CommonJS resolver only if `error.code` is ERR_MODULE_NOT_FOUND. Avoid using absolute paths in hint by: * using a parent-relative path if the specifier is a relative path * using a `pkg/x.js` format if the specifier is bare (e.g. `pkg/x`) PR-URL: #31906 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>

targos · 2020-05-07T08:39:58Z

Should we backport this to v12.x-staging? Two tests fail when I apply it.

dnlup · 2020-05-08T06:52:28Z

@targos could you post a link of the failing build? I am not sure I found the right one.

targos · 2020-05-08T07:26:39Z

There's no failing build. I did git cherry-pick 8f87d252fa9ff071a8edf941d431b6efd6cb9727 on v12.x-staging and ran the tests locally.

Here's the output:

[02:24|%  98|+ 2889|-   1]: release esm_loader_not_found_cjs_hint_bare.jslength differs.
expect=15
actual=16
patterns:
pattern = ^internal/modules/run_main\.js:.*$
pattern = ^\ \ \ \ internalBinding\('errors'\)\.triggerUncaughtException\($
pattern = ^\ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \^$
pattern = ^Error\ \[ERR_MODULE_NOT_FOUND\]:\ Cannot\ find\ module\ '.*test.*fixtures.*node_modules.*some_module.*obj'\ imported\ from\ .*test.*fixtures.*esm_loader_not_found_cjs_hint_bare\.mjs$
pattern = ^Did\ you\ mean\ to\ import\ some_module/obj\.js\?$
pattern = ^\ \ \ \ at\ finalizeResolution\ \(internal/modules/esm/resolve\.js:.*:.*\)$
pattern = ^\ \ \ \ at\ packageResolve\ \(internal/modules/esm/resolve\.js:.*:.*\)$
pattern = ^\ \ \ \ at\ moduleResolve\ \(internal/modules/esm/resolve\.js:.*:.*\)$
pattern = ^\ \ \ \ at\ Loader\.defaultResolve\ \[as\ _resolve\]\ \(internal/modules/esm/resolve\.js:.*:.*\)$
pattern = ^\ \ \ \ at\ Loader\.resolve\ \(internal/modules/esm/loader\.js:.*:.*\)$
pattern = ^\ \ \ \ at\ Loader\.getModuleJob\ \(internal/modules/esm/loader\.js:.*:.*\)$
pattern = ^\ \ \ \ at\ ModuleWrap\.<anonymous>\ \(internal/modules/esm/module_job\.js:.*:.*\)$
pattern = ^\ \ \ \ at\ link\ \(internal/modules/esm/module_job\.js:.*:.*\)\ \{$
pattern = ^\ \ code:\ 'ERR_MODULE_NOT_FOUND'$
pattern = ^\}$
outlines:
outline = (node:1321425) ExperimentalWarning: The ESM module loader is experimental.
outline = internal/modules/run_main.js:54
outline =     internalBinding('errors').triggerUncaughtException(
outline =                               ^
outline = Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/home/mzasso/git/nodejs/v12.x/test/fixtures/node_modules/some_module/obj' imported from /home/mzasso/git/nodejs/v12.x/test/fixtures/esm_loader_not_found_cjs_hint_bare.mjs
outline = Did you mean to import some_module/obj.js?
outline =     at finalizeResolution (internal/modules/esm/resolve.js:277:11)
outline =     at packageResolve (internal/modules/esm/resolve.js:608:14)
outline =     at moduleResolve (internal/modules/esm/resolve.js:655:14)
outline =     at Loader.defaultResolve [as _resolve] (internal/modules/esm/resolve.js:748:11)
outline =     at Loader.resolve (internal/modules/esm/loader.js:97:40)
outline =     at Loader.getModuleJob (internal/modules/esm/loader.js:243:28)
outline =     at ModuleWrap.<anonymous> (internal/modules/esm/module_job.js:42:40)
outline =     at link (internal/modules/esm/module_job.js:41:36) {
outline =   code: 'ERR_MODULE_NOT_FOUND'
outline = }
=== release esm_loader_not_found_cjs_hint_bare.js ===                    
Path: message/esm_loader_not_found_cjs_hint_bare.js
(node:1321425) ExperimentalWarning: The ESM module loader is experimental.
internal/modules/run_main.js:54
    internalBinding('errors').triggerUncaughtException(
                              ^

Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/home/mzasso/git/nodejs/v12.x/test/fixtures/node_modules/some_module/obj' imported from /home/mzasso/git/nodejs/v12.x/test/fixtures/esm_loader_not_found_cjs_hint_bare.mjs
Did you mean to import some_module/obj.js?
    at finalizeResolution (internal/modules/esm/resolve.js:277:11)
    at packageResolve (internal/modules/esm/resolve.js:608:14)
    at moduleResolve (internal/modules/esm/resolve.js:655:14)
    at Loader.defaultResolve [as _resolve] (internal/modules/esm/resolve.js:748:11)
    at Loader.resolve (internal/modules/esm/loader.js:97:40)
    at Loader.getModuleJob (internal/modules/esm/loader.js:243:28)
    at ModuleWrap.<anonymous> (internal/modules/esm/module_job.js:42:40)
    at link (internal/modules/esm/module_job.js:41:36) {
  code: 'ERR_MODULE_NOT_FOUND'
}
Command: out/Release/node /home/mzasso/git/nodejs/v12.x/test/message/esm_loader_not_found_cjs_hint_bare.js
[02:24|%  98|+ 2889|-   2]: release esm_loader_not_found_cjs_hint_relative....match failed
line=0
expect=^\(node:.*\)\ ExperimentalWarning:\ \-\-experimental\-loader\ is\ an\ experimental\ feature\.\ This\ feature\ could\ change\ at\ any\ time$
actual=(node:1321436) ExperimentalWarning: The ESM module loader is experimental.
=== release esm_loader_not_found_cjs_hint_relative.mjs ===                    
Path: message/esm_loader_not_found_cjs_hint_relative.mjs
(node:1321436) ExperimentalWarning: The ESM module loader is experimental.
(node:1321436) ExperimentalWarning: --experimental-loader is an experimental feature. This feature could change at any time
internal/modules/run_main.js:54
    internalBinding('errors').triggerUncaughtException(
                              ^

Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/home/mzasso/git/nodejs/v12.x/test/common/fixtures' imported from /home/mzasso/git/nodejs/v12.x/
Did you mean to import ./test/common/fixtures.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:748: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 internal/process/esm_loader.js:69:31
    at Object.initializeLoader (internal/process/esm_loader.js:74:5)
    at runMainESM (internal/modules/run_main.js:45:20)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:68:5) {
  code: 'ERR_MODULE_NOT_FOUND'
}
Command: out/Release/node --experimental-loader ./test/common/fixtures /home/mzasso/git/nodejs/v12.x/test/message/esm_loader_not_found_cjs_hint_relative.mjs
[02:30|% 100|+ 2941|-   3]: Done                                              
make[1]: *** [Makefile:296: jstest] Error 1
make: *** [Makefile:323: test] Error 2

guybedford · 2020-05-08T07:30:44Z

@targos thanks for looking into this... it seems like the difference here is the experimental modules warning on the first line. Adding this line into the output fixture for the backport should fix this and just does seem like a necessary process for any modules messages diffs while we have this difference between the versions.

targos · 2020-05-08T07:32:55Z

Aaaah of course. I didn't realize it was a message test and focused on the throw, thinking it was unexpected...
Thanks, I will fix and push. No need for a backport PR.

Run CommonJS resolver only if `error.code` is ERR_MODULE_NOT_FOUND. Avoid using absolute paths in hint by: * using a parent-relative path if the specifier is a relative path * using a `pkg/x.js` format if the specifier is bare (e.g. `pkg/x`) PR-URL: #31906 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>

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

nodejs-github-bot added the esm Issues and PRs related to the ECMAScript Modules implementation. label Feb 21, 2020

himself65 reviewed Feb 22, 2020

View reviewed changes

dnlup force-pushed the esm/better_not_found_message branch from d1d88cf to e37665c Compare February 22, 2020 14:15

dnlup changed the title ~~[WIP] esm: better error message using commonjs hint~~ esm: better error message using commonjs hint Feb 22, 2020

dnlup force-pushed the esm/better_not_found_message branch 3 times, most recently from 8ba1cc8 to 116a580 Compare February 28, 2020 11:48

dnlup force-pushed the esm/better_not_found_message branch from 116a580 to e9b1fc6 Compare March 6, 2020 12:19

BridgeAR reviewed Mar 9, 2020

View reviewed changes

test/message/esm_loader_not_found_cjs_hint.out Outdated Show resolved Hide resolved

dnlup force-pushed the esm/better_not_found_message branch from e9b1fc6 to 31ab988 Compare March 11, 2020 06:19

guybedford approved these changes Mar 12, 2020

View reviewed changes

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

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

dnlup force-pushed the esm/better_not_found_message branch from 31ab988 to 2d1faef Compare March 21, 2020 15:46

dnlup commented Mar 21, 2020

View reviewed changes

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

dnlup force-pushed the esm/better_not_found_message branch from a1b7331 to 99cacc5 Compare April 15, 2020 05:36

dnlup force-pushed the esm/better_not_found_message branch 2 times, most recently from 6901254 to f661d26 Compare April 16, 2020 06:57

MylesBorins approved these changes Apr 17, 2020

View reviewed changes

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

ljharb reviewed Apr 17, 2020

View reviewed changes

dnlup force-pushed the esm/better_not_found_message branch 2 times, most recently from e7dbfaa to 2918b22 Compare April 21, 2020 06:21

dnlup force-pushed the esm/better_not_found_message branch from 2918b22 to 6511767 Compare April 21, 2020 06:34

guybedford added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 2, 2020

guybedford closed this May 3, 2020

guybedford mentioned this pull request May 3, 2020

module: add specific error for dir import #33220

Closed

4 tasks

dnlup deleted the esm/better_not_found_message branch May 3, 2020 18:50

guybedford mentioned this pull request May 3, 2020

Better not found errors for ESM #30603

Closed

targos mentioned this pull request May 4, 2020

v14.2.0 release proposal #33232

Merged

targos added the backport-requested-v12.x label May 7, 2020

targos added the lts-watch-v12.x label May 7, 2020

targos mentioned this pull request May 8, 2020

Better errors on module not found nodejs/modules#443

Closed

targos removed backport-requested-v12.x labels May 8, 2020

targos mentioned this pull request May 13, 2020

v12.17.0 release proposal #33197

Merged

aduh95 added a commit to aduh95/node that referenced this pull request May 14, 2020

esm: improve commonjs hint on module not found

ed3dfc5

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

esm: better error message using commonjs hint #31906

esm: better error message using commonjs hint #31906

dnlup commented Feb 21, 2020 •

edited

Loading

himself65 left a comment •

edited

Loading

himself65 commented Feb 22, 2020

dnlup commented Feb 22, 2020 •

edited

Loading

guybedford left a comment

dnlup commented Mar 21, 2020 •

edited

Loading

devsnek commented Mar 22, 2020

dnlup commented Apr 15, 2020 •

edited

Loading

dnlup commented Apr 15, 2020

guybedford commented Apr 15, 2020

MylesBorins left a comment

nodejs-github-bot commented Apr 17, 2020

dnlup commented Apr 18, 2020

dnlup commented Apr 21, 2020

guybedford commented May 2, 2020

nodejs-github-bot commented May 2, 2020

guybedford commented May 3, 2020

dnlup commented May 3, 2020

targos commented May 7, 2020

dnlup commented May 8, 2020

targos commented May 8, 2020

guybedford commented May 8, 2020

targos commented May 8, 2020

esm: better error message using commonjs hint #31906

esm: better error message using commonjs hint #31906

Conversation

dnlup commented Feb 21, 2020 • edited Loading

Checklist

himself65 left a comment • edited Loading

Choose a reason for hiding this comment

himself65 commented Feb 22, 2020

dnlup commented Feb 22, 2020 • edited Loading

guybedford left a comment

Choose a reason for hiding this comment

dnlup commented Mar 21, 2020 • edited Loading

devsnek commented Mar 22, 2020

dnlup commented Apr 15, 2020 • edited Loading

dnlup commented Apr 15, 2020

guybedford commented Apr 15, 2020

MylesBorins left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Apr 17, 2020

dnlup commented Apr 18, 2020

dnlup commented Apr 21, 2020

guybedford commented May 2, 2020

nodejs-github-bot commented May 2, 2020

guybedford commented May 3, 2020

dnlup commented May 3, 2020

targos commented May 7, 2020

dnlup commented May 8, 2020

targos commented May 8, 2020

guybedford commented May 8, 2020

targos commented May 8, 2020

dnlup commented Feb 21, 2020 •

edited

Loading

himself65 left a comment •

edited

Loading

dnlup commented Feb 22, 2020 •

edited

Loading

dnlup commented Mar 21, 2020 •

edited

Loading

dnlup commented Apr 15, 2020 •

edited

Loading