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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nazrhyn
Copy link

@nazrhyn nazrhyn commented Feb 19, 2021

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 Outdated Show resolved Hide resolved
@@ -378,7 +378,7 @@ const features = {
ruleId: "no-dynamic-import",
cases: [
{
supported: null,
supported: new Range(">=12.17 <13 || >=13.2"),
Copy link
Author

@nazrhyn nazrhyn Feb 19, 2021

Choose a reason for hiding this comment

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

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."

Comment on lines +438 to +464
if (!aCase.supported) {
return true
}

if (aCase.supported instanceof Range) {
return !options.version.intersects(aCase.supported)
}

return options.version.intersects(getSemverRange(`<${aCase.supported}`))
Copy link
Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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.

Comment on lines +2506 to +2508
supported: new Range(
">=12.17 <13 || >=13.2"
).toString(),
Copy link
Author

Choose a reason for hiding this comment

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

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
Copy link

ljharb commented Feb 19, 2021

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
Copy link
Author

nazrhyn commented Feb 19, 2021

@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
Copy link

ljharb commented Feb 19, 2021

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
Copy link
Author

nazrhyn commented Feb 19, 2021

@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
Copy link

ljharb commented Feb 19, 2021

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.

Copy link
Collaborator

@sosukesuzuki sosukesuzuki left a comment

Choose a reason for hiding this comment

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

Looks good to me. /cc @mysticatea

@nazrhyn
Copy link
Author

nazrhyn commented Apr 8, 2021

/bump @mysticatea

@thernstig
Copy link

/bump @mysticatea 😆 (because this is nice)

@nazrhyn
Copy link
Author

nazrhyn commented Jun 12, 2021

Just updated from upstream to resolve conflicts.

@nazrhyn
Copy link
Author

nazrhyn commented Jun 12, 2021

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
Copy link
Author

nazrhyn commented Jul 9, 2021

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
Copy link
Contributor

kevinoid commented Jul 9, 2021

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.

Copy link
Contributor

@aladdin-add aladdin-add left a comment

Choose a reason for hiding this comment

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

LGTM

@aladdin-add
Copy link
Contributor

friendly ping @mysticatea

@thernstig
Copy link

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

@goosewobbler
Copy link

goosewobbler commented Mar 1, 2022

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
Copy link

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
Copy link

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

@nazrhyn
Copy link
Author

nazrhyn commented Mar 1, 2022

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
Copy link
Collaborator

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
Copy link

@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
Copy link

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

@ljharb
Copy link

ljharb commented Mar 23, 2022

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

@julien-f
Copy link

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

@ljharb
Copy link

ljharb commented Mar 23, 2022

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
Copy link

@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
Copy link
Author

nazrhyn commented Mar 23, 2022

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:

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
Copy link

rhansen commented Mar 23, 2022

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

@nazrhyn Yes please!

@nazrhyn
Copy link
Author

nazrhyn commented Mar 23, 2022

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
Copy link
Contributor

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

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

@rhansen
Copy link

rhansen commented Mar 24, 2022

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
Copy link
Author

nazrhyn commented Mar 24, 2022

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.

brettz9 pushed a commit to brettz9/eslint-plugin-node that referenced this pull request Jul 24, 2024
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'import()' expressions are not supported yet on Node engine >=14.5.0
10 participants