-
-
Notifications
You must be signed in to change notification settings - Fork 13
fix: Empty basePath should not be treated as cwd #120
base: main
Are you sure you want to change the base?
Conversation
In Node.js, `path.relative("")` always resolves to `process.cwd()`,
which causes problems when people use the `Linter` API in ESLint where
the `basePath` of a `ConfigArray` is set to an empty string.
This ensures that only non-empty `basePath` strings are passed to
`path.relative()`.
Refs eslint/eslint#17669
tests/config-array.test.js
Outdated
| it('should return a config when an absolute path filename matches a files pattern', () => { | ||
|
|
||
| const config = configs.getConfig('/x/foo.js'); | ||
| expect(config).to.deep.equal({ | ||
| defs: { | ||
| severity: 'error' | ||
| } | ||
| }); | ||
| }); |
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 is expected behavior in this case:
configs = new ConfigArray([
{
files: ['x/*.js'],
defs: {
severity: 'error'
}
}
], { basePath: '', schema });
configs.normalizeSync();
const config = configs.getConfig('/x/foo.js'); // `undefined` or `{ defs: { severity: 'error' } }` ?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.
Interesting question. My initial reaction is that this should return undefined, because the match is done from the left of the string, and because the pattern doesn't start with /, that isn't a match. I've added some tests to this effect.
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.
This would change how Linter works in browsers, where cwd is / (at least the way we bundle).
When I add the following to src/playground/index.js:
import { Linter } from "./node_modules/eslint/lib/linter/";
const linter = new Linter({ configType: "flat" });
const code = "foo();";
const config = [{
files: ["bar/baz.js"],
rules: {
"no-undef": 2
}
}];
const filename = "/bar/baz.js";
const result = linter.verify(code, config, filename);
console.log(JSON.stringify(result, null, 4));currently, it logs:
[
{
"ruleId": "no-undef",
"severity": 2,
"message": "'foo' is not defined.",
"line": 1,
"column": 1,
"nodeType": "Identifier",
"messageId": "undef",
"endLine": 1,
"endColumn": 4
}
]
because the config matches. However, after this change the config doesn't match and it logs:
[]
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.
To me, files: ['**/*.js'] matching '/x/foo.js' is confusing, as file patterns are supposed to be relative to something. If we don't want basePath to default to an absolute path (currently, it basically defaults to cwd), then when basePath isn't passed I'd rather expect this to work in "relative-only" mode and don't match any absolute paths.
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.
Also, note that after this change process.cwd() would still be implicitly used when basePath is a non-empty non-absolute path, which looks inconsistent with how empty basePath is treated.
const configs = new ConfigArray([
{
files: ['**/*.js'],
defs: {
severity: 'error'
}
}
], { basePath: 'foo', schema }); // works the same as if it were `path.resolve('foo')`
configs.normalizeSync();
console.log(configs.getConfig('/foo/bar.js')); // undefined
console.log(configs.getConfig(path.resolve('foo/bar.js'))); // { defs: { severity: 'error' } }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.
To me,
files: ['**/*.js']matching'/x/foo.js'is confusing, as file patterns are supposed to be relative to something. If we don't wantbasePathto default to an absolute path (currently, it basically defaults tocwd), then whenbasePathisn't passed I'd rather expect this to work in "relative-only" mode and don't match any absolute paths.
I need some context for this: are you saying this is the way things work in ESLint currently or after the change in this PR?
In Node.js,
path.relative("")always resolves toprocess.cwd(), which causes problems when people use theLinterAPI in ESLint where thebasePathof aConfigArrayis set to an empty string.This ensures that only non-empty
basePathstrings are passed topath.relative().Refs eslint/eslint#17669
cc @mdjermanovic