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

standard.lintFiles() doesn't use process.cwd() #1956

Open
regseb opened this issue Nov 29, 2023 · 5 comments · May be fixed by standard/standard-engine#329, standard/standard-engine#330 or #1959
Open

standard.lintFiles() doesn't use process.cwd() #1956

regseb opened this issue Nov 29, 2023 · 5 comments · May be fixed by standard/standard-engine#329, standard/standard-engine#330 or #1959

Comments

@regseb
Copy link

regseb commented Nov 29, 2023

Here's what I did

Files

  • package.json

    {
      "name": "testcase",
      "version": "1.0.0",
      "type": "module",
      "dependencies": {
        "standard": "17.1.0"
      }
    }
  • index.js

    import fs from "node:fs/promises";
    import process from "node:process";
    import standard from "standard";
    
    console.log(await fs.readFile("foo/bar.js", "utf8"));
    console.log(await standard.lintFiles(["foo/bar.js"]));
    
    process.chdir("foo");
    console.log(await fs.readFile("bar.js", "utf8"));
    console.log(await standard.lintFiles(["bar.js"]));
  • foo/bar.js

    var baz = 'qux'

Steps

  1. npm install
  2. node index.js

What I expected to happen

var baz = 'qux'

[
  {
    filePath: '/home/regseb/testcase/foo/bar.js',
    messages: [ [Object], [Object] ],
    suppressedMessages: [],
    errorCount: 1,
    fatalErrorCount: 0,
    warningCount: 1,
    fixableErrorCount: 0,
    fixableWarningCount: 1,
    source: "var baz = 'qux'\n",
    usedDeprecatedRules: [Getter]
  }
]
var baz = 'qux'


[
  {
    filePath: '/home/regseb/testcase/foo/bar.js',
    messages: [ [Object], [Object] ],
    suppressedMessages: [],
    errorCount: 1,
    fatalErrorCount: 0,
    warningCount: 1,
    fixableErrorCount: 0,
    fixableWarningCount: 1,
    source: "var baz = 'qux'\n",
    usedDeprecatedRules: [Getter]
  }
]

What seems to have happened

var baz = 'qux'

[
  {
    filePath: '/home/regseb/testcase/foo/bar.js',
    messages: [ [Object], [Object] ],
    suppressedMessages: [],
    errorCount: 1,
    fatalErrorCount: 0,
    warningCount: 1,
    fixableErrorCount: 0,
    fixableWarningCount: 1,
    source: "var baz = 'qux'\n",
    usedDeprecatedRules: [Getter]
  }
]
var baz = 'qux'

/home/regseb/testcase/node_modules/eslint/lib/cli-engine/file-enumerator.js:320
                    throw new NoFilesFoundError(
                          ^

NoFilesFoundError: No files matching 'bar.js' were found.
    at FileEnumerator.iterateFiles (/home/regseb/testcase/node_modules/eslint/lib/cli-engine/file-enumerator.js:320:27)
    at iterateFiles.next (<anonymous>)
    at CLIEngine.executeOnFiles (/home/regseb/testcase/node_modules/eslint/lib/cli-engine/cli-engine.js:797:48)
    at ESLint.lintFiles (/home/regseb/testcase/node_modules/eslint/lib/eslint/eslint.js:551:23)
    at StandardEngine.lintFiles (/home/regseb/testcase/node_modules/standard-engine/index.js:89:41)
    at file:///home/regseb/testcase/index.js:11:28 {
  messageTemplate: 'file-not-found',
  messageData: { pattern: 'bar.js', globDisabled: false }
}

Node.js v20.10.0

Standard doesn't use the current directory. It uses the current directory when importing "standard":

@wesleytodd
Copy link

Looks like it might be relatively simple to add cwd = process.cwd() in lib/options? Want to open a PR for that?

@regseb regseb linked a pull request Dec 1, 2023 that will close this issue
@regseb
Copy link
Author

regseb commented Dec 8, 2023

Want to open a PR for that?

I opened the pull request standard/standard-engine#329.

@rhettjay
Copy link
Member

Thanks for taking the time to submit the PR. I must apologize, I'm somewhat confused on the use case you reference in the PR. It feels like you may want to investigate spawning a child process per linter instead of changing the process dir. As @wesleytodd mentioned, adjusting the cwd in flight can get hairy because so many things can rely on it.

Nonetheless, your issue as described is currently possible by simply adjusting the cwd property on standard before linting the files. Which utilizing your sample code looks like the following:

import fs from "node:fs/promises";
import process from "node:process";
import standard from "standard";

console.log(await fs.readFile("foo/bar.js", "utf8"));
console.log(await standard.lintFiles(["foo/bar.js"]));

process.chdir("foo");
console.log(await fs.readFile("bar.js", "utf8"));
standard.cwd = process.cwd()
console.log(await standard.lintFiles(["bar.js"]));

Tested locally with node 20 and standard 17 on an intel macOS and it worked fine for me.

@regseb
Copy link
Author

regseb commented Dec 14, 2023

I worked around the bug with this code (and the directory change works):

const results = await standard.lintFiles(["bar.js"], {                       
    cwd: process.cwd(),                                                  
}); 

I've opened this issue and a PR to fix the problem in Standard so that everyone can benefit from it. And also because it's strange to pass the default value; you feel like removing it because it seems useless.

If you don't want to correct this problem, I think the documentation should be changed to reflect reality.

@rhettjay
Copy link
Member

Confirmed. I didn't understand then. The workaround (what I thought you were implying was not working properly) is what I believe should be done in this case.

it's strange to pass the default value; you feel like removing it because it seems useless.

I concede your point: we should not require a user to specify a default property. On the other hand, I remain convinced it's better practice to avoid defaulting to the process at function call time instead setting it when the class constructor is called. The existing code allows for this. In fact, I believe we would find the majority of users are not using standard in this manner anyway, but are executing it via the cli or some manner of integration. For those reasons, I'm -1 on accepting the adjustment and +1 on correcting the documentation.

I'll submit a preliminary PR, but allow time for you or others to weigh in if you feel I've missed something. Thanks again for raising this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
3 participants