fix: Get cwd on call

[regseb]

Fix standard/standard#1956

[welcome]

🙌 Thanks for opening this pull request! You're awesome.

[wesleytodd]

Personally I would have done this the other way around, set the default in the constructor. That is often helpful for debugging so folks can log it out before use. In this setup if they did that it would log undefined which would be confusing.

[regseb]

process.chdir('foo');
const engine = new StandardEngine();
process.chdir('bar');
const results = await engine.lintFiles(['baz.js']);

The documentation explains that the default value for the cwd option is process.cwd(). When the lintFiles() method is called, process.cwd() returns 'bar', but the method will use 'foo', which was the value of process.cwd() at the time of the constructor.

With this pull request, the current working directory at the time of the lintFiles() method call is used by default.

[wesleytodd]

Sure, but also what you describe is a footgun for TONS of things which rely on cwd. IMO this pattern should be avoided if at all possible (and in this case it is not only possible but easy).

[regseb]

Do you have another solution to correct this problem? You talked about setting the default in the constructor or adding cwd = process.cwd() in lib/options, but I don't see how.

[wesleytodd]

Made it as a suggestion comment. That is all I meant.

[regseb]

Your suggestion is a rollback of my commit? If I apply your two suggestions, this pull request will no longer change the source code.

[wesleytodd]

I probably should have not fired off those messages so quickly. Was split attention between a few things. Sorry about that! Looking again I am positive I just misread change as a change to the comment not the code below it. I probably need to go re-read the other issue where this came from again because clearly I didn't have all the context in my head when making this suggestion comment.

[wesleytodd]

Ok, yeah I see the original issue mentions chdir as well! I totally missed that both on first read and in following up on this. That is where I was missing context when I made the comments above.

What I meant in my original comment was to add it here but also since I did not notice you remove it from the constructor and had thought that was just a comment change (again sorry for my too quick responses).

This will teach me to fire off a quick response like that in the future.

So, to re-frame this now: it is my opinion that process.chdir in this context is not something standard-engine should respect after setting it in the constructor. Changing directory after the start of the program is almost always a footgun because so many things can rely on it and the original design here of accepting it in the constructor is a best practice (which I didn't remember in the initial issue was already being followed).

Again, really really sorry for the miss-direction here. That is totally my fault.

[regseb]

Here's my use case: I'm developing Metalint, a tool that aggregates the results of several linters, including Standard. For my tests, I'll create a temporary directory and place test files in it. Then I change the current directory and call the wrapper of Standard. But this doesn't work, because Standard fetches the files from the current directory when the tests are launched.

I tested my version of StandardEngine with my unit tests and it works.

---

If you don't want to accept my PR, you'll have to change the documentation for which current directory is used:

• standard-engine:

  {
    // ...
    cwd: '', // current working directory (default: process.cwd() when calling the StandardEngine constructor)
    // ...
  }

• standard:

  {
    // ...
    cwd: '', // current working directory (default: process.cwd() when standard is imported)
    // ...
  }

[wesleytodd]

Any way you can just create the StandardEngine class after cd'ing? Another way is just do .cwd = process.cwd(). Right?

[wesleytodd]

Also I should add, I am not the final say here in any form. A few folks have been working on what governance of this project should look like, but I think ultimately the decision is up to Feross (not going to ping since I assume he is watching most activity and will get to it when he can).

[voxpelli]

I think this would be a partial revert of #181 from 5 years ago, more specifically of d4c2916, which I'm 👍 on, this seems like a needless optimization

As @wesleytodd says there are governance discussions in standard/standard#1948 / standard/standard#1957, before those are done its a bit unclear who would make a call to change (I mean: I could merge and release it, but I rather wait. Also, I think this issue may go away in a move to ESLint 9 and flat configs as the need for standard-engine as it is today then more or less goes away)

@regseb Any reason why you use standard rather than eslint-config-standard + eslint? Personally I find the programmatic access to standard to be kind of an odd thing, that package mostly makes sense when you want to get it all installed through a single dependency. For complex cases / special cases I would go with eslint-config-standard + eslint instead, and since you already do eslint that should make it less complex for you even? With less duplicate code?

[regseb]

@regseb Any reason why you use standard rather than eslint-config-standard + eslint?

I'm developing Metalint, a tool that aggregates the results of several linters. Users configure the linters they want to use (they can use Standard or ESLint or both).

[voxpelli]

@regseb So the user themselves install standard rather than it being included by you?

[regseb]

The user must install the metalint and standard dependencies. Then, in the Metalint configuration, the user associates the *.js files with Standard.