Allow dynamic import for Node.js >=12.17 <13 || >=13.2 #256

nazrhyn · 2021-02-19T18:14:23Z

Summary

This attempts to resolve #250 by specifying the supported version for dynamic imports. I'll open some discussions on the code around some of my changes. I wasn't sure exactly what your criteria was for commit message emoji, but I tried to infer it and 💥 seemed like the right one.

Looking at your semver rules, it seems like this could require a major version, since I think I modified an existing config ("An existing config is updated."). Though, this should only make things that were previously failing pass for some Node.js versions. That's definitely up to you, though I did want to mention that I thought about it. 🤓

Changes

Extend case.supported to support explicit Range instances
Update error string for no-dynamic-import
Update tests to check around those constraints

.gitignore

nazrhyn · 2021-02-19T18:17:48Z

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

@@ -378,7 +378,7 @@ const features = {
        ruleId: "no-dynamic-import",
        cases: [
            {
-                supported: null,
+                supported: new Range(">=12.17 <13 || >=13.2"),


I thought through several ways of accomplishing this, and landed on allowing case.supported to be a Range instance. Here are my reasons:

When it's a string, the getSemverRange function is called from isNotSupportingVersion while automatically prepending a < character, to make it a negative assertion to match the Not in the function name, so I'd have to mess with that behavior. I can't change getSemverRange because it's nicely memoized and used all over the place in different ways.

I thought of making supported allow arrays, but then exactly how to combine the array values wasn't clear.

Given those things, I figured that we could just say, "If you want to specify some highly-specific range, just go ahead and make a Range and we'll use that."

nazrhyn · 2021-02-19T18:18:32Z

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

+        if (!aCase.supported) {
+            return true
+        }
+
+        if (aCase.supported instanceof Range) {
+            return !options.version.intersects(aCase.supported)
+        }
+
+        return options.version.intersects(getSemverRange(`<${aCase.supported}`))


This could be written as a single Boolean expression, but I figured expanding it was a bit more clear.

nazrhyn · 2021-02-19T18:19:02Z

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

@@ -2489,27 +2490,27 @@ ruleTester.run(
                    code: "obj.import(source)",
                    options: [{ version: "12.0.0" }],
                },
-                {
+                ...["12.17.0", "13.2.0"].map(v => ({


In both cases, I did this spread-from-map approach to keep the object DRY, but we could also just duplicate the valid and invalid cases.

nazrhyn · 2021-02-19T18:19:35Z

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

+                                supported: new Range(
+                                    ">=12.17 <13 || >=13.2"
+                                ).toString(),


I figured it was better to configure this with whatever Range's behavior is so that we didn't end up being brittle around Range when we really just wanted to test our own code.

ljharb · 2021-02-19T19:08:14Z

This handles when the syntax is supported, but it seems useful to also warn on node 13.2-13.6 where import() parses but doesn't ever succeed?

nazrhyn · 2021-02-19T19:11:34Z

@ljharb

This handles when the syntax is supported, but it seems useful to also warn on node 13.2-13.6 where import() parses but doesn't ever succeed?

Does anything else in this library differentiate between "parses" and "runs"? I guess I'm not sure.

IMO "supported" is that it works, rather than just parses, if the idea is that this should prevent someone from trying to use something that doesn't work for their Node.js version.

ljharb · 2021-02-19T19:12:43Z

I can't think of any other example, in node or browsers, of syntax that parses but doesn't function :-)

if the idea is that this should prevent someone from trying to use something that doesn't work for their Node.js version.

Right - "work" doesn't necessarily mean "parses", it likely implies "does what it's supposed to do"

nazrhyn · 2021-02-19T19:17:40Z

@ljharb

I can't think of any other example, in node or browsers, of syntax that parses but doesn't function :-)

😂 Yeah it's a bit odd. It seems "safest" to just say that it doesn't work in this case, maybe? What do you think?

ljharb · 2021-02-19T19:18:58Z

That's what I'm assuming. In the rare case where someone is supporting node 13.2, and checking if import() succeeds or not and providing a fallback, it seems like an eslint override comment would be appropriate.

sosukesuzuki

Looks good to me. /cc @mysticatea

nazrhyn · 2021-04-08T00:50:41Z

/bump @mysticatea

thernstig · 2021-05-27T15:05:02Z

/bump @mysticatea 😆 (because this is nice)

nazrhyn · 2021-06-12T14:40:34Z

Just updated from upstream to resolve conflicts.

nazrhyn · 2021-06-12T14:42:46Z

Something changed with the tests, so I'll have to go fix the test failures, too. Will have to come back to do that later.

nazrhyn · 2021-07-09T21:37:47Z

Okay I have no idea what happened with that last rebase, but we back now. All tests passing and parts of my changes not just missing.

Looking at some of the other merges, maybe @sosukesuzuki or @kevinoid could look at this and merge it? 🙏

kevinoid · 2021-07-09T23:36:00Z

Looks good to me. Worked well on the projects I tested. Thanks for working on it @nazrhyn!

Unfortunately, I'm not a member of this project, so my review probably doesn't add much weight. Hopefully the maintainers can take a look soon.

aladdin-add

LGTM

aladdin-add · 2021-08-04T06:27:25Z

friendly ping @mysticatea

thernstig · 2021-09-13T11:51:21Z

Yay awesome, can't wait for a new release including this :)

goosewobbler · 2022-03-01T16:17:20Z

Doesn't look like the friendly pings are working and this project has been abandoned. @sosukesuzuki you were the last person to merge something (9 months ago), any chance you can get this released?

thernstig · 2022-03-01T16:22:57Z

Doesn't look like the friendly pings are working and this project has been abandoned. @sosukesuzuki you were the last person to merge something (9 months ago), any chance you can get this released?

I posted this, let's see if we get some reply: mysticatea/eslint-evaluating-issues#4

goosewobbler · 2022-03-01T16:26:53Z

@thernstig good job. Loads of people using this plugin, would be a big shame to see it die.

nazrhyn · 2022-03-01T17:34:10Z

Chiming in to let you guys know I'm still here, too! If there's any movement, I will make sure it's all rebased up and tests are still passing. Thanks for all the support! ❤️

sosukesuzuki · 2022-03-02T00:12:10Z

As I previously commented, I have a permission to merge but I don't have a permission to release. I sent private DM to mysticatea but he has not reply. Maybe he doesn't have enough time for working on OSS.

thernstig · 2022-03-02T14:51:17Z

@sosukesuzuki It is fine someone does not have time to work more on OSS, no one can hold a grudge against that considering the huge free time they donated to this project. The nice thing to do to the community would however be to let more maintainers join and make more people able to release. (Possibly also move the project to an organization to get more recognition, but that is of course up to @mysticatea if he feels like it).

julien-f · 2022-03-23T09:52:26Z

@nazrhyn This plugin is no longer maintained, can you move this PR to weiran-zsd/eslint-plugin-node?

ljharb · 2022-03-23T12:43:34Z

@julien-f that’s not really something that can be safely claimed by anyone but the owner of the project.

julien-f · 2022-03-23T12:54:11Z

@ljharb True, but latest version was released two years ago and there were no replies to #300.

ljharb · 2022-03-23T13:01:32Z

And that issue links to a different fork, one by an eslint maintainer. That’s the “dangerous” part - fragmentation and the potential for malware.

julien-f · 2022-03-23T13:02:59Z

@ljharb It's the same fork 🙂

And yes, I agree fragmentation is an issue but I don't see a better solution than rallying behind the same fork.

nazrhyn · 2022-03-23T16:41:15Z

I've been thinking about this myself.

I feel like there's an amount of diligence that must be done in attempting to maintain the agency of the original author of a package, for sure. But at some point, that amount has been done. I guess the problem is that I've seen some people get very offended around the difference in opinion on what that quantity is.

If we were to try to establish a new, maintained, version of this plugin, we'd have to:

Pick a different name. (eslint-plugin-nodejs?)
Work with ESLint to update references like:
- https://eslint.org/docs/developer-guide/working-with-plugins#linting
- https://eslint.org/docs/user-guide/migrating-to-7.0.0#-nodejscommonjs-rules-have-been-deprecated
  This one is more debatable because it's kind of "point in time", but doesn't mean that people could be misdirected by reading the link to eslint-plugin-node there.
- https://eslint.org/docs/rules/no-restricted-modules
- ...and so on.
Look for other community references and make sure it's clear which one is supported.

And even if we did all of that, we still couldn't modify the README of this repository to indicate that it had been deprecated or succeeded and why. This situation is definitely something that the Node.js/NPM/GitHub communities don't have a great process for.

That said, I'd be happy to port this PR and these changes over there, if you'd like.

rhansen · 2022-03-23T19:48:03Z

That said, I'd be happy to port this PR and these changes over there, if you'd like.

@nazrhyn Yes please!

nazrhyn · 2022-03-23T22:46:20Z

That said, I'd be happy to port this PR and these changes over there, if you'd like.

@nazrhyn Yes please!

Done! 🙇

I wonder if I should just close this one, over here?

aladdin-add · 2022-03-24T04:41:27Z

I wonder if I should just close this one, over here?

agreed. the rebased branch is having many commits from the fork. 😂

rhansen · 2022-03-24T05:31:36Z

This PR is no longer useful as-is, but now that eslint-community#13 is merged, you could force update your branch back to where it was on the off chance this project comes back to life.

* Extend case.supported to support explicit Range instances * Update error string * Update tests to check around those constraints

nazrhyn · 2022-03-24T13:37:17Z

This PR is no longer useful as-is, but now that weiran-zsd#13 is merged, you could force update your branch back to where it was on the off chance this project comes back to life.

Yeah, that would be nice! Kind of annoying I'm stuck with master here. I guess this will teach me to use a branch in my forks 😂.

Reverted and fingers crossed.

nazrhyn commented Feb 19, 2021

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

nazrhyn commented Feb 19, 2021

View reviewed changes

nazrhyn mentioned this pull request Feb 19, 2021

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

Open

nazrhyn force-pushed the master branch from 9022cf2 to 4977157 Compare February 19, 2021 19:04

sosukesuzuki approved these changes Mar 13, 2021

View reviewed changes

nazrhyn force-pushed the master branch from 4977157 to 7f5f729 Compare June 12, 2021 14:39

nazrhyn force-pushed the master branch from 7f5f729 to 1480a84 Compare July 9, 2021 21:35

aladdin-add approved these changes Aug 4, 2021

View reviewed changes

voxpelli approved these changes Sep 7, 2021

View reviewed changes

nazrhyn force-pushed the master branch from 1480a84 to 1e192b1 Compare March 23, 2022 22:42

nazrhyn mentioned this pull request Mar 23, 2022

Allow dynamic import for Node.js >=12.17 <13 || >=13.2 eslint-community/eslint-plugin-n#13

Merged

💥 Allow dynamic import for Node.js >=12.17 <13 || >=13.2

8063882

* Extend case.supported to support explicit Range instances * Update error string * Update tests to check around those constraints

nazrhyn force-pushed the master branch from 1e192b1 to 8063882 Compare March 24, 2022 13:36

Allow dynamic import for Node.js >=12.17 <13 || >=13.2 #256

Are you sure you want to change the base?

Allow dynamic import for Node.js >=12.17 <13 || >=13.2 #256

Conversation

nazrhyn commented Feb 19, 2021 • edited

Summary

Changes

nazrhyn Feb 19, 2021 • edited

Choose a reason for hiding this comment

nazrhyn Feb 19, 2021

Choose a reason for hiding this comment

nazrhyn Feb 19, 2021

Choose a reason for hiding this comment

nazrhyn Feb 19, 2021

Choose a reason for hiding this comment

ljharb commented Feb 19, 2021

nazrhyn commented Feb 19, 2021

ljharb commented Feb 19, 2021 • edited

nazrhyn commented Feb 19, 2021 • edited

ljharb commented Feb 19, 2021

sosukesuzuki left a comment

Choose a reason for hiding this comment

nazrhyn commented Apr 8, 2021

thernstig commented May 27, 2021

nazrhyn commented Jun 12, 2021

nazrhyn commented Jun 12, 2021

nazrhyn commented Jul 9, 2021

kevinoid commented Jul 9, 2021

aladdin-add left a comment

Choose a reason for hiding this comment

aladdin-add commented Aug 4, 2021

thernstig commented Sep 13, 2021

goosewobbler commented Mar 1, 2022 • edited

thernstig commented Mar 1, 2022

goosewobbler commented Mar 1, 2022

nazrhyn commented Mar 1, 2022

sosukesuzuki commented Mar 2, 2022

thernstig commented Mar 2, 2022

julien-f commented Mar 23, 2022

ljharb commented Mar 23, 2022

julien-f commented Mar 23, 2022

ljharb commented Mar 23, 2022

julien-f commented Mar 23, 2022

nazrhyn commented Mar 23, 2022 • edited

rhansen commented Mar 23, 2022

nazrhyn commented Mar 23, 2022

aladdin-add commented Mar 24, 2022

rhansen commented Mar 24, 2022

nazrhyn commented Mar 24, 2022

nazrhyn commented Feb 19, 2021 •

edited

nazrhyn Feb 19, 2021 •

edited

ljharb commented Feb 19, 2021 •

edited

nazrhyn commented Feb 19, 2021 •

edited

goosewobbler commented Mar 1, 2022 •

edited

nazrhyn commented Mar 23, 2022 •

edited