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

fix: Empty basePath should not be treated as cwd #120

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

nzakas
Copy link
Contributor

@nzakas nzakas commented Oct 27, 2023

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

cc @mdjermanovic

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
Comment on lines 923 to 931
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'
}
});
});
Copy link
Contributor

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' } }` ?

Copy link
Contributor Author

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.

Copy link
Contributor

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:

[]

Copy link
Contributor

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.

Copy link
Contributor

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' } }

Copy link
Contributor Author

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.

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants