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

dynamic import can't be used in the REPL #19570

Closed
targos opened this issue Mar 24, 2018 · 13 comments
Closed

dynamic import can't be used in the REPL #19570

targos opened this issue Mar 24, 2018 · 13 comments
Assignees
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. repl Issues and PRs related to the REPL subsystem.

Comments

@targos
Copy link
Member

targos commented Mar 24, 2018

$ ./node --experimental-modules
> (node:14483) ExperimentalWarning: The ESM module loader is experimental.
> import('fs').then(console.log, console.log)
Promise {
  <pending>,
  domain:
   Domain {
     domain: null,
     _events:
      { removeListener: [Function: updateExceptionCapture],
        newListener: [Function: updateExceptionCapture],
        error: [Function: debugDomainError] },
     _eventsCount: 3,
     _maxListeners: undefined,
     members: [] } }
> { TypeError [ERR_INVALID_URL]: Invalid URL: repl
    at onParseError (internal/url.js:226:17)
    at parse (internal/url.js:235:3)
    at new URL (internal/url.js:318:5)
    at normalizeReferrerURL (internal/process/esm_loader.js:18:10)
    at setImportModuleDynamicallyCallback (internal/process/esm_loader.js:51:37)
    at process._tickCallback (internal/process/next_tick.js:114:7) input: 'repl' }
@targos targos added repl Issues and PRs related to the REPL subsystem. esm Issues and PRs related to the ECMAScript Modules implementation. labels Mar 24, 2018
@devsnek
Copy link
Member

devsnek commented Mar 24, 2018

i have this working in a branch, i'm just waiting on a few things before I open a pr

@thw0rted
Copy link
Contributor

thw0rted commented Apr 24, 2018

Is the non-function version of import (import {Foo} from "./bar.mjs";) supposed to work on the REPL? When I type that, I get a continuation prompt (...) as if I'd just typed the open brace of a function definition or conditional. (Node 8.11.1 LTS)

Does the fix @devsnek alluded to address this? Maybe it already works in a newer Node?

ETA: Nope, not in newer Node either -- I got 9.11.1 installed and now instead of the continuation prompt, I get

> import {Testing} from "./mod.mjs";
import {Testing} from "./mod.mjs";
       ^

SyntaxError: Unexpected token {

> import Testing from "./mod.mjs";
import Testing from "./mod.mjs";
       ^^^^^^^

SyntaxError: Unexpected identifier

Should I open a separate issue about parsing the REPL with the MJS / ESM loader, or is that addressed here?

@devsnek
Copy link
Member

devsnek commented Apr 24, 2018

@thw0rted that won't be supported any time soon.. the repl runs as a script parse goal. @bmeck has a proposal for tc39 for a "repl parse goal" somewhere which would allow it

@thw0rted
Copy link
Contributor

Sorry, I'm a little new to Node. I guess what I'm really looking for is, can I use a .mjs from the REPL at all, and if so, what's the proper syntax? (And if not, is this the issue where that capability is going to get added?)

@devsnek
Copy link
Member

devsnek commented Apr 24, 2018

@thw0rted no you cannot right now, but maybe in the future, and we're already tracking adding stuff like that 👍

@rubys
Copy link
Member

rubys commented Jul 31, 2018

I've investigated this a bit, but I'm not quite ready to do a pull request.

This can be made to work with the following patch:

diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js
index 016c0c5e23..7f0e62ac99 100644
--- a/lib/internal/bootstrap/node.js
+++ b/lib/internal/bootstrap/node.js
@@ -303,7 +303,8 @@
               checkScriptSyntax(code, '[stdin]');
             } else {
               process._eval = code;
-              evalScript('[stdin]');
+              const homeUrl = `file://${process.cwd().replace(/\\/g, '/')}/`;
+              evalScript(homeUrl + '[stdin]');
             }
           });
         }
diff --git a/lib/repl.js b/lib/repl.js
index 92c90de7bb..4e7885ac46 100644
--- a/lib/repl.js
+++ b/lib/repl.js
@@ -616,7 +616,8 @@ function REPLServer(prompt,
     const evalCmd = self[kBufferedCommandSymbol] + cmd + '\n';
 
     debug('eval %j', evalCmd);
-    self.eval(evalCmd, self.context, 'repl', finish);
+    const homeUrl = `file://${process.cwd().replace(/\\/g, '/')}/`;
+    self.eval(evalCmd, self.context, homeUrl + 'repl', finish);
 
     function finish(e, ret) {
       debug('finish', e, ret);

The reason for two changes is that there are separate code paths is that stdin from a file/pipe and stdin from a tty are separate code paths.

This change causes 5 existing tests to fail, many because they verify stack traces and the file name will show up there. And one because there is no cwd, so defensive code would need to be added.

https://gist.github.com/rubys/b79c692831b2a9a6dea4897c3d121f7b contains a test, which mostly passes when run individually, and mostly fails when run as a part of jstest. The failure in both cases is /Users/rubys/git/node/out/Release/node: bad option: --experimental-modules.

Once this is made to work, and once #21805 lands, then we will be in a position where the SyntaxError when parsing import statements will be passed to acorn and parsed. If it parses correctly as an import statement and if --experimental-repl-await is specified, then the equivalent await import() call can be substituted for the input.

@devsnek
Copy link
Member

devsnek commented Jul 31, 2018

there is a bunch of work that needs to be done to ModuleWrap and ContextifyScript first... I have the changes locally, just haven't finished them yet. (I realize I said that four months ago but it's actually close now 😓)

@rubys
Copy link
Member

rubys commented Jul 31, 2018

@devsnek anything I can do to help?

@devsnek
Copy link
Member

devsnek commented Jul 31, 2018

@rubys I'll open a PR with my current changes tomorrow and see what people think

@rubys
Copy link
Member

rubys commented Aug 17, 2018

@devsnek ping?

@devsnek
Copy link
Member

devsnek commented Aug 17, 2018

whoops i completely forgot about this... rebasing branch now and then i'll throw up a pr

@Jamesernator
Copy link

Jamesernator commented Nov 6, 2019

This is currently mostly working, in particular dynamic import can be used to import a file but you need to use a relative path to the folder's parent directory.

e.g. If you're in directory /foo/bar/baz then to import /foo/bar/baz/qux.js you need to import './baz/qux.js' rather than just ./qux.js even though ./baz/ is the directory you're already in.

@devsnek
Copy link
Member

devsnek commented Nov 6, 2019

The fix is basically to set the referrer to process.cwd() + '/repl'.

coreyfarrell added a commit to coreyfarrell/node that referenced this issue Nov 30, 2019
The ESM loader does not accept a directory as the referrer, it requires
a path within the directory.  Add `/repl` to ensure relative dynamic
imports can succeed.

Fixes: nodejs#19570
@bcoe bcoe closed this as completed in 49fb529 Dec 1, 2019
targos pushed a commit that referenced this issue Dec 1, 2019
The ESM loader does not accept a directory as the referrer, it requires
a path within the directory.  Add `/repl` to ensure relative dynamic
imports can succeed.

Fixes: #19570

PR-URL: #30609
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Coe <[email protected]>
MylesBorins pushed a commit that referenced this issue Jan 12, 2020
The ESM loader does not accept a directory as the referrer, it requires
a path within the directory.  Add `/repl` to ensure relative dynamic
imports can succeed.

Fixes: #19570

PR-URL: #30609
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Coe <[email protected]>
BethGriggs pushed a commit that referenced this issue Feb 6, 2020
The ESM loader does not accept a directory as the referrer, it requires
a path within the directory.  Add `/repl` to ensure relative dynamic
imports can succeed.

Fixes: #19570

PR-URL: #30609
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Coe <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants