search

AtomLinter / linter-eslint-node

ESLint plugin for Atom/Pulsar Linter (v8 and above)
https://web.pulsar-edit.dev/packages/linter-eslint-node
MIT License
4 stars 3 forks source link

Get some more tests working #4

Closed savetheclocktower closed 2 years ago

savetheclocktower commented 2 years ago

Added support for a couple of things and uncommented the associated tests.

BTW, I went back to the Message text (rule-id) style to match what linter-eslint does.

savetheclocktower commented 2 years ago

That failing test in CI is passing for me locally, so that'll be a fun one to figure out.

UziTech commented 2 years ago

Looks like it is finding an .eslintignore somewhere.

You can ignore the commitlint checks. That is only needed when we start using semantic-release.

savetheclocktower commented 2 years ago

Ah, it fails when I run it via atom --test, at least. That gets me somewhere.

savetheclocktower commented 2 years ago

OK, I think I found it.

If I start the worker from within the Atom GUI, its process.cwd() is /, and it finds no .eslintignore.

If I start the worker from atom --test, its process.cwd() is my shell working directory, and it finds the .eslintignore in the root of the project. I can “fix” it from the CLI by doing a process.chdir('/') at the top of the script, but obviously that's silly, and not an actual fix. And I'm not sure why the worker script inherits $PWD in one scenario but not the other.

This is annoying to me because I think ESLint's Node API docs could've made it clearer that an instance of ESLint is valid only in the directory it was instantiated in. The fallback-to-our-own-eslint thing merely makes the issue plain: right now, two files from very different places in the filesystem will end up reusing the same ESLint instances because they rely on the same fallback resolved ESLint module path.

Tomorrow I'll redo the caching so that it's not keyed on that path, but rather on path.dirname(filePath). If I lint /Users/andrew/foo/one.js, and a minute later I lint /Users/andrew/foo/two.js, the latter linting can (almost certainly) safely reuse the ESLints we instantiated for one.js. We can even save the process.chdir by passing the cwd option into the ESLint constructor.

Since we'll almost always be dealing with projects, and thus lots of files living in the same directories with other files, it's probably still worth it to do the caching. But if it causes even one more headache after all this, I'll have to take a hard look at it.