Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

repl: support --loader option in builtin REPL #33437

Closed
wants to merge 1 commit into from

Conversation

targos
Copy link
Member

@targos targos commented May 16, 2020

Fixes: #33435

I'll try to write a test if there are no objections to the change.

/cc @nodejs/modules-active-members

@targos
Copy link
Member Author

targos commented May 16, 2020

With the example from the issue:

$ echo 'console.log("hooks executing")' > hooks.mjs
$ ./node --loader ./hooks.mjs
(node:1190852) ExperimentalWarning: --experimental-loader is an experimental feature. This feature could change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
hooks executing
Welcome to Node.js v15.0.0-pre.
Type ".help" for more information.

With a wrong path:

$ ./node --loader ./hooks.js
(node:1190903) ExperimentalWarning: --experimental-loader is an experimental feature. This feature could change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
internal/main/repl.js:70
    internalBinding('errors').triggerUncaughtException(
                              ^

Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/home/mzasso/git/nodejs/node/hooks.js' imported from /home/mzasso/git/nodejs/node/
    at finalizeResolution (internal/modules/esm/resolve.js:284:11)
    at moduleResolve (internal/modules/esm/resolve.js:662:10)
    at Loader.defaultResolve [as _resolve] (internal/modules/esm/resolve.js:752:11)
    at Loader.resolve (internal/modules/esm/loader.js:97:40)
    at Loader.getModuleJob (internal/modules/esm/loader.js:242:28)
    at Loader.import (internal/modules/esm/loader.js:177:28)
    at internal/process/esm_loader.js:55:31
    at Object.initializeLoader (internal/process/esm_loader.js:60:5)
    at internal/main/repl.js:37:13 {
  code: 'ERR_MODULE_NOT_FOUND'

With error in the loader code:

$ echo 'throw new Error("test")' > hooks.mjs
$ ./node --loader ./hooks.mjs
(node:1190987) ExperimentalWarning: --experimental-loader is an experimental feature. This feature could change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
file:///home/mzasso/git/nodejs/node/hooks.mjs:1
throw new Error("test")
      ^

Error: test
    at file:///home/mzasso/git/nodejs/node/hooks.mjs:1:7
    at ModuleJob.run (internal/modules/esm/module_job.js:138:23)
    at async Loader.import (internal/modules/esm/loader.js:178:24)
    at async internal/process/esm_loader.js:55:9

lib/internal/main/repl.js Outdated Show resolved Hide resolved
lib/internal/main/repl.js Outdated Show resolved Hide resolved
lib/internal/main/repl.js Outdated Show resolved Hide resolved
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine with adding this feature. It would be good in general to support esm better in the REPL.

lib/internal/main/repl.js Outdated Show resolved Hide resolved
@devsnek
Copy link
Member

devsnek commented May 16, 2020

Maybe this is too heavy for this pr, but refactoring internal/process/esm_loader to gate exposing the loader instance behind a function which also ensures the loader is properly initialized would be nice.

@BridgeAR
Copy link
Member

Ping @targos

@targos
Copy link
Member Author

targos commented May 29, 2020

Updated with a different approach. PTAL.


exports.loadESM = async function loadESM(callback) {
try {
await initializeLoader();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should probably gate this on ESMLoader being undefined

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you mean add something like assert(ESMLoader === undefined) ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah that works.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't. There's a default loader defined above, and removing it breaks things.

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

targos added a commit that referenced this pull request Jul 14, 2020
Fixes: #33435

PR-URL: #33437
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@targos
Copy link
Member Author

targos commented Jul 14, 2020

Landed in 05539c1

@targos targos closed this Jul 14, 2020
@targos targos deleted the repl-loader branch July 14, 2020 13:21
MylesBorins pushed a commit that referenced this pull request Jul 14, 2020
Fixes: #33435

PR-URL: #33437
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins MylesBorins mentioned this pull request Jul 14, 2020
MylesBorins pushed a commit that referenced this pull request Jul 16, 2020
Fixes: #33435

PR-URL: #33437
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
cjihrig pushed a commit that referenced this pull request Jul 23, 2020
Fixes: #33435

PR-URL: #33437
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@codebytere
Copy link
Member

@targos could you please backport this to v12.x?

targos added a commit to targos/node that referenced this pull request Sep 28, 2020
Fixes: nodejs#33435

PR-URL: nodejs#33437
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
addaleax pushed a commit that referenced this pull request Sep 28, 2020
Fixes: #33435

PR-URL: #33437
Backport-PR-URL: #35394
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@codebytere codebytere mentioned this pull request Oct 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

node --loader for REPL (without entrypoint) does not execute loader
6 participants