feat!: add support for Node.js ESM resolution #4

giltayar · 2022-01-22T07:32:48Z

Adding support for ESM Node.js resolution, for example for packages that have no main in package.json.

This required me to bring in a package that is similar to resolve, but supports modern Node.js ESM resolution. I researched and the only one that fit this purpose was import-meta-resolve.

Unfortunately, the package is an ESM only package, and has no CJS import! And even more unfortunately, I couldn't find an "async" entry point in the ESLint plugin so that I could await import it. I asked the maintainer of the package to create a CJS entry point (that is also synchronous), but they advised me to build it locally as a CJS, using some bundler (see wooorm/import-meta-resolve#2).

This is why there is the dependency on esbuild and the shenanigans I do in the update script, whereby I bundle the import-meta-resolve package into a file in lib. I could think of no other way.

Fortunately, it works! And I added some tests in this regard.

This fix should probably resolve LOTS of issues surrounding modern package resolution.

giltayar · 2022-01-22T07:38:27Z

BTW, something weird happened when I ran npm run build. All the docs changed the name of the rule from n to node in the documentation.

Was that on purpose? Did I miss something?

What I did was revert the doc changes before commiting, but if that was erroneous, please tell me and I'll add them back in another commit.

(also please see #5, where I believe this plugin should be renamed)

aladdin-add

looks great, I just have a few minor questions/suggestions. Thanks for the PR! 👍

scripts/convert-pure-esm-to-cjs.js

lib/util/import-target.js

lib/rules/no-hide-core-modules.js

lib/util/import-target.js

aladdin-add · 2022-01-23T16:36:53Z

tests/lib/rules/no-missing-import.js

+        {
+            filename: fixture("test.js"),
+            code: "import something from 'esm-module/sub';",
+        },


Can you add some invalid cases?

Oh, shit. The failing test opened a hornet's nest of problems. Will get back to you in a few... :-)

OK, that "invalid case" test failed because it was valid. The code allows things that Node.js ESM just doesn't allow. So I changed it, and now other tests that passed do NOT pass, because they allowed things that are not allowed! So this is turning into quite the work for me. It'll take me a few days. Will definitely get back to you on this one.

Sorry, I should have been clearer. I mean not using the right esm export. e.g.

{"exports": {".": "src/main.js"}}

import mod from "esm-module/src/main"; // => error import mod from "esm-module/src/main.js"; // => error import mod from "esm-module/src/not-main.js"; // => error

aladdin-add · 2022-01-23T16:38:23Z

tests/lib/rules/no-missing-import.js

+        {
+            filename: fixture("test.js"),
+            code: "import something from 'esm-module/sub';",
+        },


Just to confirm: is the import() supported too?

Yes. It goes through the exact same mechanism. I'll add a test, just for documentation purposes.

aladdin-add · 2022-01-25T03:54:47Z

scripts/update.js

@@ -4,6 +4,8 @@
 */
 "use strict"

+require("./convert-pure-esm-to-cjs")


this could unexpectedly update the import-meta-resolve, as we are not using package-lock.
I would suggest running a separate script to upgrade it.

Co-authored-by: 唯然 <[email protected]>

giltayar · 2022-01-28T16:20:36Z

OK, big change from previous change. The problem in my previous code was that while it passed new ESM resolutions (like resolutions that use the new package.json export field), it did not invaliate import-s that are now invalid. For example, import-s that do not have extensions are invalid now from Node v12, and should have failed. They did not. The new commits do this.

Note that this means that supporting tryExtensions in import has no meaning anymore, and thus was removed from import rules (don't worry, it's still there for require rules).

This could theoretically break existing code, and thus should be a SEMVER_MAJOR change, but only theoretically, because anyway it would break the Node.js code that is running.

aladdin-add · 2022-01-29T08:14:47Z

it looks like md files were re-formatted in the new pushed commit, no intentional?

giltayar · 2022-01-30T09:18:01Z

@aladdin-add Huh. Strange. Not intentional. I did some changes in the md files to remove the tryExtensions in the import rules, but that's it. I'll look into it. Please ignore for now?

scripts/update.js

aladdin-add · 2022-02-07T02:56:33Z

lib/rules/file-extension-in-import.js

-                properties: {
-                    tryExtensions: getTryExtensions.schema,
-                },
+                properties: {},


❗️removing the tryExtensions will be a breaking change.

BePo65 · 2022-02-16T09:19:35Z

Just to be sure that I understood the current state:

'eslint-plugin-n' V14.0.0 does not support import from packages that have an 'exports' field only
this pr would change this, so that packages that have an 'exports' field only can be linted
I just have to wait :-)

My current problem is a program that uses got with version 12, which is an esm only package.

Thanks for working on this topic.

giltayar · 2022-02-17T05:06:31Z

@BePo65 exactly! And what prompted this PR for me is that suddenly there are packages out there with no main field!

aladdin-add · 2022-02-21T09:56:03Z

I was wondering if it can be released in a non-major version.

it is indeed a breaking change to remove tryExtensions supports.
but the option was not actually working as node's esm(technically a bug 😂).

giltayar · 2022-02-22T06:05:54Z

Your call! I tend to be strict in my interpretation of semver. But that's me!

From my POV, I don't care. We'd really love this PR to be merged, because we are a heavy ESM shop, and this is interfering with our ability to use the nice features of ESM like imports and self-reference of a module. The current code (v10 compatible) kinda works OK, but misses in edge cases, and allows things not allowed in modern ESM (like extensionless paths).

aladdin-add · 2022-02-23T02:30:29Z

released in v15.0.0, thanks for the awesome work!

aladdin-add self-requested a review January 23, 2022 15:09

aladdin-add reviewed Jan 23, 2022

View reviewed changes

aladdin-add reviewed Jan 25, 2022

View reviewed changes

giltayar and others added 3 commits January 28, 2022 18:00

add support for Node.js ESM resolution

31132cf

Update lib/rules/no-hide-core-modules.js

10a7f0b

Co-authored-by: 唯然 <[email protected]>

Update lib/util/import-target.js

b072e22

giltayar force-pushed the esm-resolution branch from b14f43f to 09108d7 Compare January 28, 2022 16:17

invalidate ESM imports that were valid in Node 10

2dd5161

giltayar force-pushed the esm-resolution branch from 09108d7 to 2dd5161 Compare February 4, 2022 09:55

aladdin-add self-requested a review February 5, 2022 12:28

aladdin-add reviewed Feb 7, 2022

View reviewed changes

scripts/update.js Outdated Show resolved Hide resolved

aladdin-add reviewed Feb 7, 2022

View reviewed changes

aladdin-add changed the title ~~add support for Node.js ESM resolution~~ feat!: add support for Node.js ESM resolution Feb 7, 2022

Update scripts/update.js

5ce184c

aladdin-add approved these changes Feb 14, 2022

View reviewed changes

aladdin-add requested a review from voxpelli February 14, 2022 03:34

giltayar mentioned this pull request Feb 14, 2022

no-missing-imports does not support "exports" in package.json mysticatea/eslint-plugin-node#255

Open

aladdin-add merged commit d24be36 into eslint-community:master Feb 23, 2022

giltayar deleted the esm-resolution branch February 23, 2022 06:04

piranna mentioned this pull request Jun 18, 2022

add support for Node.js ESM resolution import-js/eslint-plugin-import#2480

Closed

SBoudrias mentioned this pull request Jan 20, 2023

Build script to setup ESM/CJS module exports SBoudrias/Inquirer.js#1185

Closed

SBoudrias mentioned this pull request Jan 20, 2023

no-missing-imports don't support dependencies subpaths #75

Closed

github-actions bot mentioned this pull request Feb 27, 2024

chore(release-please): release 17.0.0-4 #193

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: add support for Node.js ESM resolution #4

feat!: add support for Node.js ESM resolution #4

giltayar commented Jan 22, 2022

giltayar commented Jan 22, 2022

aladdin-add left a comment

aladdin-add Jan 23, 2022

giltayar Jan 24, 2022

giltayar Jan 24, 2022

giltayar Jan 24, 2022

aladdin-add Jan 25, 2022 •

edited

Loading

aladdin-add Jan 23, 2022

giltayar Jan 24, 2022

aladdin-add Jan 25, 2022

giltayar commented Jan 28, 2022 •

edited

Loading

aladdin-add commented Jan 29, 2022

giltayar commented Jan 30, 2022

aladdin-add Feb 7, 2022

BePo65 commented Feb 16, 2022

giltayar commented Feb 17, 2022

aladdin-add commented Feb 21, 2022

giltayar commented Feb 22, 2022

aladdin-add commented Feb 23, 2022

feat!: add support for Node.js ESM resolution #4

feat!: add support for Node.js ESM resolution #4

Conversation

giltayar commented Jan 22, 2022

giltayar commented Jan 22, 2022

aladdin-add left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aladdin-add Jan 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giltayar commented Jan 28, 2022 • edited Loading

aladdin-add commented Jan 29, 2022

giltayar commented Jan 30, 2022

Choose a reason for hiding this comment

BePo65 commented Feb 16, 2022

giltayar commented Feb 17, 2022

aladdin-add commented Feb 21, 2022

giltayar commented Feb 22, 2022

aladdin-add commented Feb 23, 2022

aladdin-add Jan 25, 2022 •

edited

Loading

giltayar commented Jan 28, 2022 •

edited

Loading