-
Notifications
You must be signed in to change notification settings - Fork 140
Conversation
I think the issue is that Atom uses an old version of node (12.14.1) and I don't think it has the ability to load es modules which eslint plugins are starting to use. |
@@ -435,7 +435,7 @@ describe('The eslint provider for Linter', () => { | |||
checkAfter(newMessages) | |||
}) | |||
|
|||
it('allows ignoring all fixable rules while typing', async () => { | |||
xit('allows ignoring all fixable rules while typing', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have disabled these tests because as far as I can see there is no way to detect what rules are loaded and fixable for a given file 😬
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about ESLint#calculateConfigForFile
?
FWIW, although Node 12 did start ESM support, ESLint required a higher minor version per this comment eslint/eslint#14023 (comment) . It seems Atom is really slow to update the bundled Node version due to being deterred from Electron upgrades (e.g., see atom/atom#22765 ). Even the Electron version on Though less than ideal, how feasible would it be to bundle a higher Node version and use that in its own process or what not? It seems it may be a while otherwise, and not supporting the latest ESLint, especially after some additional minor releases already having occurred, really does not seem viable for a modern IDE to keep apace. |
One way we could get around the node version issue would be to run eslint cli and parse the output. |
I have actually just stopped trying to adapt this (because supporting the options was rather hard). I instead wrote my own version from scratch here is the code if you want it in a gist (https://gist.github.com/scagood/061ebc869de679abc1324ab184cdd4a0). It does not have any options because I couldnt be bothered to add to something I was not sure I was going to share it. There is one major issue in the one I wrote, that being it assumes that 'node' is executable from atom (which it wont be if you use nvm). |
Thanks @scagood! Your script solved it for me 🙏 FYI: |
🤔 I guess you're using nvm or something similar? @UziTech Do you want me to PR the code I linked, which drops support for |
@scagood it might be better to create a different linter with your code so we don't break people who use older eslint and need the options. If you want we could host it in AtomLinter. |
Yes, please, would be great if this could be released even as a new and less-featured package. I think Atom itself really needs this. |
I like the approach that @scagood's gist takes: instead of consuming ESLint's JS API within Atom's own version of Node, consume it with whatever version the project is using, and run our own worker script within that environment, passing messages back and forth between the two processes. I'm stuck on how to reliably detect which version of Node the user needs, and I've been trying to think through it. When I wrote an ESLint bundle for TextMate many years ago, I could just rely on the user to tell me which one to use by defining a Here are the options that I can think of — I know someone else will be able to think of more:
This saga has made me curious about whether other packages have had this problem, and how they've solved it. But I can't find anything obvious, plus it's not the easiest thing to google. |
I just discovered I'm wrong about Option 2: I figured the |
Yes, ESLint 8 itself dropped support for Node before version 12 and specifically requires at least 12.22/14.17/16(see eslint/eslint#14023 though see esp. eslint/eslint#14023 (comment) on the exact versions or https://github.com/eslint/eslint/blob/main/README.md#installation-and-usage for confirmation in the docs.) Not sure how important is it to support older versions of Node with those versions having been end-of-lifed, and much other modern tooling has dropped older Node support as well, but if the reason is to allow switching to nvm for testing of packages which support older versions, I still think a broken linter for that scenario would be superior to the present situation of it never being able to work even under the more common scenario of using a non end-of-lifed Node version. If you really need it though, could you check the |
The problem is this, @brettz9 (and whoever else is curious; I'll over-explain just to get all possible readers on the same page): Imagine you've got a local NPM project where you've installed ESLint 7. The Now you upgrade to version 8 and your linting has stopped working. You think it's because ESLint 8 removed To be honest, this was always a bit of a hack, and could theoretically have broken at any point in the past, since there's no guarantee that a random package pulled from a user's own environment would work with Atom's specific version of Node. So it's safer to switch to a model where Atom calls a process and that process runs the linting and reports back the results, because then that process isn't bound to Atom's old version of Node. The best version of Node to use is whichever one the user was using when they ran That's not even getting into the NVM-specific stuff! To iterate the installed NVM versions, as you suggest, would require access to the But that's just because I typically open my project by
Also, maybe the user prefers Volta. I hear it's quite good. The way to cut this Gordian knot is to ask the user to specify the path to the right version of Node, but that can vary from project to project, and Atom has no built-in concept of per-project settings. So I have no idea what to do. I can think of solutions which would mostly work for me, but not necessarily for other macOS users, much less Windows users (lord, how does any of this work on Windows?). |
Thanks for the info... Plan to look over more carefully, but although ESLint is moving toward ESM support, with some of its dependencies offering such exports--but those are in addition to CommonJS, and ESLint itself actually doesn't even offer ESM itself. It only offers CommonJS still. The new plugin format--which I have not yet gotten into using yet myself--was supposed to perhaps support reading of ESM plugins, but I haven't confirmed that myself. Perhaps the issue was that ESLint is using |
Yeah, I oversimplified — some plugins are starting to use ES modules, not ESLint itself. @scagood hit his roadblock above when he found that the plugins he was testing with weren't loading. I looked into how VSCode's ESLint extension handles this, and it's as I suspected. Since VSCode supports per-project settings, the user can define I am hesitant to propose new third-party dependencies to fix problems, but I'll bring this up anyway: project-config hooks into plumbing that already exists in Atom to make per-project settings possible. (It doesn't have a lot of existing users, but neither does any other Atom package that didn't already exist five years ago.) Perhaps the answer could be “use this setting to tell us where Node is, and install this other optional package if that setting needs to change from project to project.” |
If so, I also hope using the approach of Thanks for all your helpful explanations, by the way. And would be great in any case, to see something allowing ESLint 8 access again! |
Sadly, I was wrong about that; since the shebang line of that
I agree, but I think that NVM would need to meet us halfway here; this is the downside of having a version manager that is so heavily reliant on shell voodoo. In theory, NVM could create its own binary that, when invoked, would traverse upward from This is mainly why I moved from RVM to rbenv way back when, and sometimes lessons need to be re-learned. |
Would users adding a |
That's an option, sure, and maybe it's a way forward for people who don't want to install the project-config package. My vote would be to support both, since VSCode has normalized concept of per-project config; hypothetically, if other packages were to need their own per-project config settings, I'd want those all to live in the same place rather than have each package choose some random dotfile to stick their settings into. Whereas project-config would want a file that could be directly applied over the default config file: {
"linter-eslint-v8": {
"nodeBin": "/Users/andrew/.volta/bin/node"
}
} …a {
"nodeBin": "/Users/andrew/.volta/bin/node"
} Only snag is what happens when both configs are present and disagree. The answer could just be to pick an order in which they're consumed and stick with it; I don't think it matters which order, as long as it's consistent. |
Description
This is my first pass attempt to support eslint v8 (#1440).
I have made this PR because I have hit 2 road blocks:
Any help fixing these two problems would be greatly appreciated! (I just need to be pointed in roughly the correct direction if that is possible)