-
-
Notifications
You must be signed in to change notification settings - Fork 10
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!: correctly detect if file is outside base path on Windows #59
base: main
Are you sure you want to change the base?
Changes from 11 commits
1c6d147
f39fc53
93fbcef
7df1675
5d84435
e408f2b
aa25167
a73982e
2d7af8a
133ee16
9d129e7
a783ba1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,2 @@ | ||
package-lock = false | ||
@jsr:registry=https://npm.jsr.io | ||
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -7,6 +7,7 @@ | |||
//----------------------------------------------------------------------------- | ||||
|
||||
import assert from "node:assert"; | ||||
import path from "node:path"; | ||||
import { | ||||
fixupRule, | ||||
fixupPluginRules, | ||||
|
@@ -133,7 +134,7 @@ describe("@eslint/backcompat", () => { | |||
const linter = new Linter(); | ||||
const code = "var foo = () => 123; function bar() { return 123; }"; | ||||
const messages = linter.verify(code, config, { | ||||
filename: "test.js", | ||||
filename: path.resolve("test.js"), | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When the
|
||||
}); | ||||
|
||||
assert.deepStrictEqual( | ||||
|
@@ -203,7 +204,7 @@ describe("@eslint/backcompat", () => { | |||
const code = | ||||
"var foo = () => 123; function bar() { return 123; }"; | ||||
const messages = linter.verify(code, config, { | ||||
filename: "test.js", | ||||
filename: path.resolve("test.js"), | ||||
}); | ||||
|
||||
assert.deepStrictEqual( | ||||
|
@@ -283,7 +284,7 @@ describe("@eslint/backcompat", () => { | |||
const code = | ||||
"var foo = () => 123; function bar() { for (const x of y) { foo(); } }"; | ||||
const messages = linter.verify(code, config, { | ||||
filename: "test.js", | ||||
filename: path.resolve("test.js"), | ||||
}); | ||||
|
||||
assert.deepStrictEqual( | ||||
|
@@ -507,7 +508,7 @@ describe("@eslint/backcompat", () => { | |||
}, | ||||
}, | ||||
{ | ||||
filename: "test.js", | ||||
filename: path.resolve("test.js"), | ||||
}, | ||||
); | ||||
|
||||
|
@@ -641,7 +642,7 @@ describe("@eslint/backcompat", () => { | |||
const code = | ||||
"var foo = () => 123; function bar() { return 123; }"; | ||||
const messages = linter.verify(code, fixupConfigRules(config), { | ||||
filename: "test.js", | ||||
filename: path.resolve("test.js"), | ||||
}); | ||||
|
||||
assert.deepStrictEqual( | ||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
/* | ||
* Replace import specifiers in "dist" modules to use the bundled versions of "@jsr/std__path". | ||
* | ||
* In "dist/cjs/index.cjs": | ||
* - '@jsr/std__path/posix' → './std__path/posix.cjs' | ||
* - '@jsr/std__path/windows' → './std__path/windows.cjs' | ||
* | ||
* In "dist/esm/index.js": | ||
* - '@jsr/std__path/posix' → './std__path/posix.js' | ||
* - '@jsr/std__path/windows' → './std__path/windows.js' | ||
*/ | ||
|
||
import { readFile, writeFile } from "node:fs/promises"; | ||
|
||
async function replaceInFile(file, search, replacement) { | ||
let text = await readFile(file, "utf-8"); | ||
text = text.replace(search, replacement); | ||
await writeFile(file, text); | ||
} | ||
|
||
const SEARCH_REGEXP = /'@jsr\/std__path\/(.+?)'/gu; | ||
|
||
await Promise.all([ | ||
replaceInFile("dist/cjs/index.cjs", SEARCH_REGEXP, "'./std__path/$1.cjs'"), | ||
replaceInFile("dist/esm/index.js", SEARCH_REGEXP, "'./std__path/$1.js'"), | ||
]); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
export default [ | ||
{ | ||
input: "../../node_modules/@jsr/std__path/posix/mod.js", | ||
output: [ | ||
{ | ||
file: "./dist/cjs/std__path/posix.cjs", | ||
format: "cjs", | ||
}, | ||
{ | ||
file: "./dist/esm/std__path/posix.js", | ||
format: "esm", | ||
}, | ||
], | ||
}, | ||
{ | ||
input: "../../node_modules/@jsr/std__path/windows/mod.js", | ||
output: [ | ||
{ | ||
file: "./dist/cjs/std__path/windows.cjs", | ||
format: "cjs", | ||
}, | ||
{ | ||
file: "./dist/esm/std__path/windows.js", | ||
format: "esm", | ||
}, | ||
], | ||
}, | ||
]; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,7 +7,8 @@ | |
// Imports | ||
//------------------------------------------------------------------------------ | ||
|
||
import path from "node:path"; | ||
import * as posixPath from "@jsr/std__path/posix"; | ||
import * as windowsPath from "@jsr/std__path/windows"; | ||
import minimatch from "minimatch"; | ||
import createDebug from "debug"; | ||
|
||
|
@@ -89,6 +90,9 @@ const CONFIG_WITH_STATUS_UNCONFIGURED = Object.freeze({ | |
status: "unconfigured", | ||
}); | ||
|
||
// Match two leading dots followed by a slash or the end of input. | ||
const EXTERNAL_PATH_REGEX = /^\.\.(\/|$)/u; | ||
|
||
/** | ||
* Wrapper error for config validation errors that adds a name to the front of the | ||
* error message. | ||
|
@@ -348,15 +352,11 @@ function normalizeSync(items, context, extraConfigTypes) { | |
* matcher. | ||
* @param {Array<string|((string) => boolean)>} ignores The ignore patterns to check. | ||
* @param {string} filePath The absolute path of the file to check. | ||
* @param {string} relativeFilePath The relative path of the file to check. | ||
* @param {string} relativeFilePath The path of the file to check relative to the base path, | ||
* using slash ("/") as a separator. | ||
* @returns {boolean} True if the path should be ignored and false if not. | ||
*/ | ||
function shouldIgnorePath(ignores, filePath, relativeFilePath) { | ||
// all files outside of the basePath are ignored | ||
if (relativeFilePath.startsWith("..")) { | ||
return true; | ||
} | ||
Comment on lines
-356
to
-358
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This check is already performed in |
||
|
||
return ignores.reduce((ignored, matcher) => { | ||
if (!ignored) { | ||
if (typeof matcher === "function") { | ||
|
@@ -387,19 +387,13 @@ function shouldIgnorePath(ignores, filePath, relativeFilePath) { | |
* Determines if a given file path is matched by a config based on | ||
* `ignores` only. | ||
* @param {string} filePath The absolute file path to check. | ||
* @param {string} basePath The base path for the config. | ||
* @param {string} relativeFilePath The path of the file to check relative to the base path, | ||
* using slash ("/") as a separator. | ||
* @param {Object} config The config object to check. | ||
* @returns {boolean} True if the file path is matched by the config, | ||
* false if not. | ||
*/ | ||
function pathMatchesIgnores(filePath, basePath, config) { | ||
/* | ||
* For both files and ignores, functions are passed the absolute | ||
* file path while strings are compared against the relative | ||
* file path. | ||
*/ | ||
const relativeFilePath = path.relative(basePath, filePath); | ||
|
||
function pathMatchesIgnores(filePath, relativeFilePath, config) { | ||
return ( | ||
Object.keys(config).filter(key => !META_FIELDS.has(key)).length > 1 && | ||
!shouldIgnorePath(config.ignores, filePath, relativeFilePath) | ||
|
@@ -412,19 +406,12 @@ function pathMatchesIgnores(filePath, basePath, config) { | |
* is present then we match the globs in `files` and exclude any globs in | ||
* `ignores`. | ||
* @param {string} filePath The absolute file path to check. | ||
* @param {string} basePath The base path for the config. | ||
* @param {string} relativeFilePath The path of the file to check relative to the base path. | ||
* @param {Object} config The config object to check. | ||
* @returns {boolean} True if the file path is matched by the config, | ||
* false if not. | ||
*/ | ||
function pathMatches(filePath, basePath, config) { | ||
/* | ||
* For both files and ignores, functions are passed the absolute | ||
* file path while strings are compared against the relative | ||
* file path. | ||
*/ | ||
const relativeFilePath = path.relative(basePath, filePath); | ||
|
||
function pathMatches(filePath, relativeFilePath, config) { | ||
// match both strings and functions | ||
function match(pattern) { | ||
if (isString(pattern)) { | ||
|
@@ -499,6 +486,15 @@ function assertExtraConfigTypes(extraConfigTypes) { | |
} | ||
} | ||
|
||
/** | ||
* Returns path-handling implementations for Unix or Windows, depending on a given absolute path. | ||
* @param {string} path The absolute path to check. | ||
* @returns {typeof import("@jsr/std__path")} Path-handling implementations for the specified path. | ||
*/ | ||
function getPathImpl(path) { | ||
return path.startsWith("/") ? posixPath : windowsPath; | ||
} | ||
|
||
//------------------------------------------------------------------------------ | ||
// Public Interface | ||
//------------------------------------------------------------------------------ | ||
|
@@ -524,7 +520,7 @@ export class ConfigArray extends Array { | |
* @param {Iterable|Function|Object} configs An iterable yielding config | ||
* objects, or a config function, or a config object. | ||
* @param {Object} options The options for the ConfigArray. | ||
* @param {string} [options.basePath=""] The path of the config file | ||
* @param {string} [options.basePath=""] The absolute path of the config file directory. | ||
* @param {boolean} [options.normalized=false] Flag indicating if the | ||
* configs have already been normalized. | ||
* @param {Object} [options.schema] The additional schema | ||
|
@@ -599,6 +595,12 @@ export class ConfigArray extends Array { | |
} else { | ||
this.push(configs); | ||
} | ||
|
||
// On Windows, `path.relative()` returns an absolute path when given two paths on different drives. | ||
// The namespaced base path is useful to make sure that calculated relative paths are always relative. | ||
// On Unix, it is identical to the base path. | ||
this.namespacedBasePath = | ||
basePath && getPathImpl(this.basePath).toNamespacedPath(basePath); | ||
} | ||
|
||
/** | ||
|
@@ -802,11 +804,21 @@ export class ConfigArray extends Array { | |
return cache.get(filePath); | ||
} | ||
|
||
// Select path-handling implementations depending on the specified file path. | ||
const path = getPathImpl(filePath); | ||
|
||
// check to see if the file is outside the base path | ||
|
||
const relativeFilePath = path.relative(this.basePath, filePath); | ||
const namespacedFilePath = path.toNamespacedPath(filePath); | ||
|
||
// If base path is not specified, relative paths cannot be built. | ||
const relativeFilePath = ( | ||
this.namespacedBasePath | ||
? path.relative(this.namespacedBasePath, namespacedFilePath) | ||
: namespacedFilePath | ||
).replaceAll(path.SEPARATOR, "/"); | ||
|
||
if (relativeFilePath.startsWith("..")) { | ||
if (EXTERNAL_PATH_REGEX.test(relativeFilePath)) { | ||
debug(`No config for file ${filePath} outside of base path`); | ||
|
||
// cache and return result | ||
|
@@ -842,12 +854,12 @@ export class ConfigArray extends Array { | |
this.forEach((config, index) => { | ||
if (!config.files) { | ||
if (!config.ignores) { | ||
debug(`Anonymous universal config found for ${filePath}`); | ||
debug(`Universal config found for ${filePath}`); | ||
matchingConfigIndices.push(index); | ||
return; | ||
} | ||
|
||
if (pathMatchesIgnores(filePath, this.basePath, config)) { | ||
if (pathMatchesIgnores(filePath, relativeFilePath, config)) { | ||
debug( | ||
`Matching config found for ${filePath} (based on ignores: ${config.ignores})`, | ||
); | ||
|
@@ -883,7 +895,7 @@ export class ConfigArray extends Array { | |
// check that the config matches without the non-universal files first | ||
if ( | ||
nonUniversalFiles.length && | ||
pathMatches(filePath, this.basePath, { | ||
pathMatches(filePath, relativeFilePath, { | ||
files: nonUniversalFiles, | ||
ignores: config.ignores, | ||
}) | ||
|
@@ -897,7 +909,7 @@ export class ConfigArray extends Array { | |
// if there wasn't a match then check if it matches with universal files | ||
if ( | ||
universalFiles.length && | ||
pathMatches(filePath, this.basePath, { | ||
pathMatches(filePath, relativeFilePath, { | ||
files: universalFiles, | ||
ignores: config.ignores, | ||
}) | ||
|
@@ -912,7 +924,7 @@ export class ConfigArray extends Array { | |
} | ||
|
||
// the normal case | ||
if (pathMatches(filePath, this.basePath, config)) { | ||
if (pathMatches(filePath, relativeFilePath, config)) { | ||
debug(`Matching config found for ${filePath}`); | ||
matchingConfigIndices.push(index); | ||
matchFound = true; | ||
|
@@ -1020,16 +1032,27 @@ export class ConfigArray extends Array { | |
isDirectoryIgnored(directoryPath) { | ||
assertNormalized(this); | ||
|
||
const relativeDirectoryPath = path | ||
.relative(this.basePath, directoryPath) | ||
.replace(/\\/gu, "/"); | ||
// Select path-handling implementations depending on the specified directory path. | ||
const path = getPathImpl(directoryPath); | ||
|
||
const namespacedDirectoryPath = path.toNamespacedPath(directoryPath); | ||
|
||
// If base path is not specified, relative paths cannot be built. | ||
const relativeDirectoryPath = ( | ||
this.namespacedBasePath | ||
? path.relative( | ||
this.namespacedBasePath, | ||
namespacedDirectoryPath, | ||
) | ||
: namespacedDirectoryPath | ||
).replaceAll(path.SEPARATOR, "/"); | ||
|
||
// basePath directory can never be ignored | ||
if (relativeDirectoryPath === "") { | ||
return false; | ||
} | ||
|
||
if (relativeDirectoryPath.startsWith("..")) { | ||
if (EXTERNAL_PATH_REGEX.test(relativeDirectoryPath)) { | ||
return true; | ||
} | ||
|
||
|
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.
Would end users need to add this to their
.npmrc
files?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.
I've tried using
npm pack
-ed version of config-array from this branch in eslint/eslint, andnpm install
fails with: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.
I don't think we can expect users to change their npm settings so that they can download packages from JSR. We could distribute
@std/path
in a separate directory insideconfig-array
and import it from there. Or we could use the URL of the tarball from JSR to specify the dependecy in package.json:Either way, we will lose the magic of the caret
^
to pick newer versions automatically. I checked the JSR docs at https://jsr.io/docs/using-packages but I didn't find any recommendations on how to use their packages with npm apart from registering JSR in .npmrc.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.
Can one of you open an issue on JSR for this? I can't imagine they didn't encounter this before, but before we spin our wheels, it would be nice to verify if we're doing things the right way.
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.
I opened a discussion here: jsr-io/jsr#701