eslint-plugin-node
eslint-plugin-node copied to clipboard
mysticatea
Allow dynamic import for Node.js >=12.17 <13 || >=13.2
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.supportedto support explicitRangeinstances - Update error string for
no-dynamic-import - Update tests to check around those constraints
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?
@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.
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"
@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?
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.
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.
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? 🙏
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.
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?
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: https://github.com/mysticatea/eslint-evaluating-issues/issues/4
@thernstig good job. Loads of people using this plugin, would be a big shame to see it die.
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! ❤️
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.
@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).
@nazrhyn This plugin is no longer maintained, can you move this PR to weiran-zsd/eslint-plugin-node?
@julien-f that’s not really something that can be safely claimed by anyone but the owner of the project.
@ljharb True, but latest version was released two years ago and there were no replies to #300.
And that issue links to a different fork, one by an eslint maintainer. That’s the “dangerous” part - fragmentation and the potential for malware.
@ljharb It's the same fork :slightly_smiling_face:
And yes, I agree fragmentation is an issue but I don't see a better solution than rallying behind the same fork.
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.
That said, I'd be happy to port this PR and these changes over there, if you'd like.
@nazrhyn Yes please!
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?
I wonder if I should just close this one, over here?
agreed. the rebased branch is having many commits from the fork. 😂
This PR is no longer useful as-is, but now that https://github.com/weiran-zsd/eslint-plugin-node/pull/13 is merged, you could force update your branch back to where it was on the off chance this project comes back to life.
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.