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

'import()' expressions are not supported yet on Node engine >=14.5.0 #250

Open
Tzahile opened this issue Dec 15, 2020 · 13 comments · Fixed by eslint-community/eslint-plugin-n#13 · May be fixed by #256
Open

'import()' expressions are not supported yet on Node engine >=14.5.0 #250

Tzahile opened this issue Dec 15, 2020 · 13 comments · Fixed by eslint-community/eslint-plugin-n#13 · May be fixed by #256

Comments

@Tzahile
Copy link

Tzahile commented Dec 15, 2020

Hello,
I'm using Node 14 and all of my project is managed as a module ("type": "module" in package.json).
Until now, using everywhere imports and exports, I didn't see this error.

Lately I added a dynamic import to some new code like the following:

if(process.env.USE_NEW_CODE) {
  const { RunNewCode } = await import("./controllers/new/code.js");
  RunNewCode();
}

Suddenly I got 'import()' expressions are not supported yet error.

I have ecmaVersion field set to 2021,
and "engines": { "node": ">=14.5.0" }
According to the README of this rule, it should be enough

Node 14 fully supports ESM

Thanks in advance!

@nazrhyn
Copy link

nazrhyn commented Dec 28, 2020

I just ran into this, myself. I took at look at the rule, and we have:

/lib/rules/no-unsupported-features/es-syntax.js

const features = {
    /* ... */
    //--------------------------------------------------------------------------
    // ES2020
    //--------------------------------------------------------------------------
    /* ... */
    dynamicImport: {
        ruleId: "no-dynamic-import",
        cases: [
            {
                supported: null,
                messageId: "no-dynamic-import",
            },
        ],
    },
}

As @Tzahile said, Node 14 supports ECMAScript Modules without a flag. Are we just missing the correct supported string instead of null?

Looking at the history at the official documentation (https://nodejs.org/docs/latest-v15.x/api/esm.html), it looks like the command line flag requirement was dropped as early as Node.js 12.17.0 and 13.2.0.

image

I suppose it does depend on what you guys consider to be "supported", though. While the command line flag was removed a while ago, the feature only became "stable" as of version 15.3.0, which you can also see in that table. I suppose, in my opinion, features that work without a flag are supported enough to not have a linting rule complain about their use.

Thoughts?

@nazrhyn
Copy link

nazrhyn commented Jan 25, 2021

Incidentally, I'd be happy to make a PR for this, but I don't want to just submit one without any chance of it getting merged.

@gamedevsam
Copy link

I also ran into this issue, please make a PR, someone will merge it!

@TomerAberbach
Copy link

I think you may be able to revert a5f3ab2, which reverted 0b0c2aa, which added supported versions for ESM and dynamic import. Although, I think it could specify "12.17.0" instead of "13.2.0".

I'm not sure why the original commit was reverted. @mysticatea was there an issue with the original commit?

@ljharb
Copy link

ljharb commented Feb 13, 2021

The semver range for node ESM support without a flag is ^12.17 || ^13.7 || >= 14. import() appears to work as of node v13.3 (but it parses in v13.0-v13.2).

@Krinkle
Copy link

Krinkle commented Feb 14, 2021

The semver range for node ESM support without a flag is ^12.17 || ^13.7 || >= 14. import() appears to work as of node v13.3 (but it parses in v13.0-v13.2).

Based on my own testing, I found that it parses without flag since Node 10 (and I expect Node 8 as well, but not verified).

nobody@fresh-6d854270c900:/qunit$ node --version
v10.15.2
nobody@fresh-6d854270c900:/qunit$ node
> function foo() { import('./bar.js'); }
undefined

Note no flags passed, and no syntax error. Upon reaching such a statement, naturally, a run-time error is found:

> foo();
(node:10) UnhandledPromiseRejectionWarning: Error: Not supported
    at foo (repl:1:18)
    at repl:1:1
    at Script.runInThisContext (vm.js:96:20)
    at REPLServer.defaultEval (repl.js:332:29)

Based on this, I was able to add support for ESM imports in QUnit whilst upholding Node 10 support, and without any complexity such as duplicate files or multiple entry point wrappers. Mocha has done the same. And both of these projects are passing CI with Node 10 without experimental flags (and both make sure the statement is not generally reached when under Node 12.17).

Perhaps I have missed a corner case where this fails on Node 10, in which case I'd very much like to know so that we can defend against that issue! (Apologies if this is about something different than what this issue is about.)

@ljharb
Copy link

ljharb commented Feb 14, 2021

It'd be reasonable for this plugin to warn on it when it doesn't work tho, as opposed to just when it doesn't parse.

@Krinkle
Copy link

Krinkle commented Feb 14, 2021

@ljharb I took me by surprise because the rule refers to itself as es-syntax which, unless documented otherwise, I would think warns on a would-be syntax error. Effectively a protective layer atop eslintrc.parserOptions.ecmaVersion. Developers tend to have to set this to a relatively high version when some syntax is implemented in Node/V8 earlier than the rest of a given spec, thus leading to false negatives for other syntax introduced that version.

I don't specifically have a preference for a rule that checks only syntax, nor do I have a preference to it actually helping understand spec-compliant runtime behaviour. Having just one rule that checks whatever we think is most useful, seems fine by me. Perhaps the author already meant for the rule to do more than check syntax, I don't know. The tool would be equally useful to me whichever way this goes. I'm merely raising that I found its behaviour surprising, and wanted to make sure the author/maintainers check if that is indeed the way they want it to behave.

@nazrhyn
Copy link

nazrhyn commented Feb 19, 2021

Okay all, here it is:
#256

I ended up doing a small refactor that allows more-complex semver strings for rule cases that is hopefully not too controversial. Let me know what you guys think, and hopefully someone notices it and wants to merge it!

@nazrhyn
Copy link

nazrhyn commented Mar 12, 2021

@mysticatea Any thoughts on this?

asurkov added a commit to camperaid/watest that referenced this issue Jun 14, 2021
note: dynamicImport eslint rule is caused by eslint-plugin-node bug mysticatea/eslint-plugin-node#250
@alexander-akait
Copy link

/cc @ljharb @mysticatea friendly ping

@ljharb
Copy link

ljharb commented Sep 17, 2021

I don’t maintain this plugin; no point pinging me.

@nazrhyn
Copy link

nazrhyn commented Mar 24, 2022

To anyone following this issue, the change in #256 has been merged over in https://github.com/weiran-zsd/eslint-plugin-node as eslint-community#13.

It looks like, until there's any movement on this repository and we hear from @mysticatea, we're trying to keep this alive over there.

travi added a commit to semantic-release/semantic-release that referenced this issue Sep 17, 2022
travi added a commit to semantic-release/semantic-release that referenced this issue Sep 17, 2022
MadLittleMods added a commit to matrix-org/matrix-viewer that referenced this issue Apr 21, 2023
MadLittleMods added a commit to matrix-org/matrix-viewer that referenced this issue Apr 21, 2023
Charlie-Crowdstrike added a commit to Charlie-Crowdstrike/eslint-config-crowdstrike-node that referenced this issue Jul 25, 2024
Charlie-Crowdstrike added a commit to Charlie-Crowdstrike/eslint-config-crowdstrike-node that referenced this issue Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants