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: display dynamic import version in error message. #48129

Merged
merged 33 commits into from
Jun 11, 2023

Conversation

hemanth
Copy link
Contributor

@hemanth hemanth commented May 22, 2023

Fix for #48084

 ./out/Release/node
Welcome to Node.js v21.0.0-pre.
Type ".help" for more information.
> import {foo, bar} from 'mo';
import {foo, bar} from 'mo';
^^^^^^

Uncaught:
SyntaxError: Cannot use import statement inside the Node.js REPL, alternatively use dynamic import: const {foo,bar} = await import('mo');
image

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. repl Issues and PRs related to the REPL subsystem. labels May 22, 2023
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

Can you please remove the unrelated spacing changes, and ensure we have a test for all types of static import statements as well as tests for when there are more than one statement (e.g. const a=1;import 'url';const b=2).

@hemanth
Copy link
Contributor Author

hemanth commented May 23, 2023

@aduh95 I ran make lint-js-fix, looks better now?

const a=1;import 'url';const b=2 that's a good catch, pretty sure that would bomb, will have a look.

update: ha! will fix it.

>const a=1;import 'url';const b=2
const a=1;import 'url';const b=2
          ^^^^^^

Uncaught:
SyntaxError: Cannot use import statement inside the Node.js REPL, alternatively use dynamic import: const  = await import('url');

Meanwhile, any idea, why this could happen? Looks like a parse error?

> import comeOn from \'fhqwhgads\'
Uncaught [SyntaxError: Expecting Unicode escape sequence \uXXXX (1:20)
] {
  pos: 20,
  loc: Position { line: 1, column: 20 },
  raisedAt: 20
}

@aduh95
Copy link
Contributor

aduh95 commented May 23, 2023

import comeOn from \'fhqwhgads\' is not valid ECMAScript (mind the backslashes), so it's expected that it produces a parse error, or am I missing something?

@hemanth
Copy link
Contributor Author

hemanth commented May 23, 2023

On v20.1.0 we see:

❯ node
Welcome to Node.js v20.1.0.
Type ".help" for more information.
> import comeOn from \'fhqwhgads\'
import comeOn from \'fhqwhgads\'
^^^^^^

Uncaught:
SyntaxError: Cannot use import statement inside the Node.js REPL, alternatively use dynamic import

^ That should be because it doesn't even parse till \'fhqwhgads\' so the expected behavior with the new messaging would be?

@hemanth
Copy link
Contributor Author

hemanth commented May 23, 2023

Works in runkit, because it interpolates the string well?

image

@hemanth
Copy link
Contributor Author

hemanth commented May 25, 2023

@aduh95 How come the test passes for:

{
  send: 'import "foo";',
  expect: ''
}
❯ tools/test.py test/parallel/test-repl-*
[00:03|% 100|+  72|-   0]: Done                                               

All tests passed.

@hemanth
Copy link
Contributor Author

hemanth commented May 27, 2023

@aduh95 internal/deps/acorn/acorn/dist/acorn and internal/deps/acorn/acorn-walk/dist/walk have different implementations compared what is on npm.

Currently stuck with:

Uncaught TypeError: baseVisitor[type] is not a function
    at c (node:internal/deps/acorn/acorn-walk/dist/walk:45:24)
    at Object.ancestor (node:internal/deps/acorn/acorn-walk/dist/walk:48:7)
    at toDynamicImport (node:repl:238:13)
    at Domain.debugDomainError (node:repl:713:62)
    at Domain.emit (node:events:511:28)
    at Domain.emit (node:domain:489:12)
    at finish (node:repl:949:22)
    at finishExecution (node:repl:571:7)
    at REPLServer.defaultEval (node:repl:660:7)
    at bound (node:domain:433:15)

Parser(options, input, startPos) param positions are different...

@aduh95
Copy link
Contributor

aduh95 commented May 27, 2023

That's unlikely, we use npm to download new versions.

"$NODE" "$NPM" install --global-style --no-bin-links --ignore-scripts "acorn@$NEW_VERSION"

@hemanth
Copy link
Contributor Author

hemanth commented May 27, 2023

acron#L9 reads constructor(options, input, startPos) their readme interface reads parse(input, options) ha, maybe it is parse vs Parser.

Also, everything works fine in runkit gist.

@hemanth
Copy link
Contributor Author

hemanth commented May 27, 2023

Ah, indeed it was Parser vs parse.

  code: 'ERR_ASSERTION',
  actual: "SyntaxError: Cannot use import statement inside the Node.js REPL, alternatively use dynamic import: const comeOn = await import('fhqwhgads');",
  expected: /^SyntaxError: .* dynamic import: const comeOn = await import('fhqwhgads');/,
  operator: 'match'

RE is not being respected? /^SyntaxError: .* dynamic import:..

Enhance the REPL message for static import error message.

```
> import {foo, bar} from 'moo';
import {foo, bar} from 'moo';
^^^^^^

Uncaught:
SyntaxError: .* dynamic import: const {foo,bar} = await import('moo');
```
@hemanth
Copy link
Contributor Author

hemanth commented May 27, 2023

Build says: Error: The operation was canceled. @aduh95 is there a keyword to re-trigger CI?

image

Also, finally looks like we have all the variants covered:

image

@aduh95
Copy link
Contributor

aduh95 commented May 27, 2023

If you push a commit to get rid of all the unrelated space changes, that should trigger a new CI run

@hemanth
Copy link
Contributor Author

hemanth commented May 27, 2023 via email

@aduh95
Copy link
Contributor

aduh95 commented May 27, 2023

The spaces changes came in from lint-fix I presume.

That's unlikely, but anyway, you would need to get rid of the unrelated changes to move this forward.

lib/repl.js Outdated Show resolved Hide resolved
lib/repl.js Outdated Show resolved Hide resolved
test/parallel/test-repl.js Show resolved Hide resolved
lib/repl.js Outdated Show resolved Hide resolved
lib/repl.js Outdated Show resolved Hide resolved
lib/repl.js Outdated Show resolved Hide resolved
lib/repl.js Outdated Show resolved Hide resolved
lib/repl.js Outdated Show resolved Hide resolved
lib/repl.js Outdated Show resolved Hide resolved
hemanth and others added 2 commits May 31, 2023 12:23
Co-authored-by: Jordan Harband <ljharb@gmail.com>
Co-authored-by: Jordan Harband <ljharb@gmail.com>
@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Jun 10, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 10, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Jun 11, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 11, 2023
@nodejs-github-bot nodejs-github-bot merged commit 0c875bb into nodejs:main Jun 11, 2023
61 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 0c875bb

@hemanth
Copy link
Contributor Author

hemanth commented Jun 11, 2023

🥳 Thank you @aduh95 and @ljharb 🤓

RafaelGSS pushed a commit that referenced this pull request Jul 3, 2023
Enhance the REPL message for static import error message.

```
> import {foo, bar} from 'moo';
import {foo, bar} from 'moo';
^^^^^^

Uncaught:
SyntaxError: .* dynamic import: const {foo,bar} = await import('moo');
```

PR-URL: #48129
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Jul 3, 2023
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
Enhance the REPL message for static import error message.

```
> import {foo, bar} from 'moo';
import {foo, bar} from 'moo';
^^^^^^

Uncaught:
SyntaxError: .* dynamic import: const {foo,bar} = await import('moo');
```

PR-URL: nodejs#48129
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
Enhance the REPL message for static import error message.

```
> import {foo, bar} from 'moo';
import {foo, bar} from 'moo';
^^^^^^

Uncaught:
SyntaxError: .* dynamic import: const {foo,bar} = await import('moo');
```

PR-URL: nodejs#48129
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
ruyadorno pushed a commit that referenced this pull request Aug 29, 2023
Enhance the REPL message for static import error message.

```
> import {foo, bar} from 'moo';
import {foo, bar} from 'moo';
^^^^^^

Uncaught:
SyntaxError: .* dynamic import: const {foo,bar} = await import('moo');
```

PR-URL: #48129
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@ruyadorno ruyadorno mentioned this pull request Aug 29, 2023
@ruyadorno
Copy link
Member

hi @hemanth this commit is breaking tests on v18.x-staging branch and will need manual backport in order to land on v18.

@ruyadorno ruyadorno added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Sep 1, 2023
@hemanth
Copy link
Contributor Author

hemanth commented Sep 1, 2023

Hey @ruyadorno Ill try to backport it, thanks for highlighting!

targos pushed a commit that referenced this pull request Nov 10, 2023
Enhance the REPL message for static import error message.

```
> import {foo, bar} from 'moo';
import {foo, bar} from 'moo';
^^^^^^

Uncaught:
SyntaxError: .* dynamic import: const {foo,bar} = await import('moo');
```

PR-URL: #48129
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
Enhance the REPL message for static import error message.

```
> import {foo, bar} from 'moo';
import {foo, bar} from 'moo';
^^^^^^

Uncaught:
SyntaxError: .* dynamic import: const {foo,bar} = await import('moo');
```

PR-URL: nodejs/node#48129
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
Enhance the REPL message for static import error message.

```
> import {foo, bar} from 'moo';
import {foo, bar} from 'moo';
^^^^^^

Uncaught:
SyntaxError: .* dynamic import: const {foo,bar} = await import('moo');
```

PR-URL: nodejs/node#48129
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants