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 windows support #214

Merged
merged 11 commits into from
Dec 24, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/quick-hats-roll.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"eslint-plugin-import-x": minor
---

Improve windows support
1 change: 1 addition & 0 deletions .gitattributes
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* text=auto eol=lf
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ jobs:
os:
- macos-latest
- ubuntu-latest
# - windows-latest
- windows-latest
node:
- 18
- 20
Expand Down
4 changes: 3 additions & 1 deletion src/rules/no-extraneous-dependencies.ts
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,9 @@ function testConfig(config: string[] | boolean | undefined, filename: string) {
}
// Array of globs.
return config.some(
c => minimatch(filename, c) || minimatch(filename, path.resolve(c)),
c =>
minimatch(filename, c) ||
minimatch(filename, path.resolve(c).replaceAll(path.sep, '/')),
SukkaW marked this conversation as resolved.
Show resolved Hide resolved
)
}

Expand Down
8 changes: 5 additions & 3 deletions src/rules/no-restricted-paths.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ const containsPath = (filepath: string, target: string) => {

function isMatchingTargetPath(filename: string, targetPath: string) {
if (isGlob(targetPath)) {
const mm = new Minimatch(targetPath)
const mm = new Minimatch(targetPath.replaceAll(path.sep, '/'))
return mm.match(filename)
}

Expand Down Expand Up @@ -171,13 +171,15 @@ export = createRule<[Options?], MessageId>({
) {
let isPathException: ((absoluteImportPath: string) => boolean) | undefined

const mm = new Minimatch(absoluteFrom)
const mm = new Minimatch(absoluteFrom.replaceAll(path.sep, '/'))
const isPathRestricted = (absoluteImportPath: string) =>
mm.match(absoluteImportPath)
const hasValidExceptions = zoneExcept.every(it => isGlob(it))

if (hasValidExceptions) {
const exceptionsMm = zoneExcept.map(except => new Minimatch(except))
const exceptionsMm = zoneExcept.map(
except => new Minimatch(except.replaceAll(path.sep, '/')),
)
isPathException = (absoluteImportPath: string) =>
exceptionsMm.some(mm => mm.match(absoluteImportPath))
}
Expand Down
5 changes: 3 additions & 2 deletions src/rules/no-unassigned-import.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,14 @@ function testIsAllow(

const filePath =
// a node module
source[0] !== '.' && source[0] !== '/'
source[0] !== '.' && source[0] !== path.sep
? source
: path.resolve(path.dirname(filename), source) // get source absolute path

return globs.some(
glob =>
minimatch(filePath, glob) || minimatch(filePath, path.resolve(glob)),
minimatch(filePath, glob) ||
minimatch(filePath, path.resolve(glob).replaceAll(path.sep, '/')),
)
}

Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

48 changes: 22 additions & 26 deletions test/rules/no-absolute-path.spec.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
import path from 'node:path'

import { RuleTester as TSESLintRuleTester } from '@typescript-eslint/rule-tester'
import type { TestCaseError as TSESLintTestCaseError } from '@typescript-eslint/rule-tester'

Expand All @@ -18,8 +16,6 @@ const ABSOLUTE_ERROR: TSESLintTestCaseError<
messageId: 'absolute',
}

const absolutePath = (testPath: string) => path.join(__dirname, testPath)

ruleTester.run('no-absolute-path', rule, {
valid: [
tValid({ code: 'import _ from "lodash"' }),
Expand Down Expand Up @@ -66,72 +62,72 @@ ruleTester.run('no-absolute-path', rule, {
],
invalid: [
tInvalid({
code: `import f from "${absolutePath('/foo')}"`,
filename: absolutePath('/foo/bar/index.js'),
code: `import f from "/foo"`,
filename: '/foo/bar/index.js',
errors: [ABSOLUTE_ERROR],
output: 'import f from ".."',
}),
tInvalid({
code: `import f from "${absolutePath('/foo/bar/baz.js')}"`,
filename: absolutePath('/foo/bar/index.js'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite understand this change, it doesn't cover Windows absolute paths.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not possible to do an absolute path import on Windows - or at least, I couldn't work out how to make one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nodejs/node#31710

file:// protocol can be used for Windows absolute path, but of course we maybe not handling file:// protocol previously, same as other protocols like http:// or https://.

But in case that file:// is only for absolute local path like previous, it could be an improvement.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, using a file protocol path seems like a different question to a posix absolute path - or at least an additional feature to be added to this rule.

code: `import f from "/foo/bar/baz.js"`,
filename: '/foo/bar/index.js',
errors: [ABSOLUTE_ERROR],
output: 'import f from "./baz.js"',
}),
tInvalid({
code: `import f from "${absolutePath('/foo/path')}"`,
filename: absolutePath('/foo/bar/index.js'),
code: `import f from "/foo/path"`,
filename: '/foo/bar/index.js',
errors: [ABSOLUTE_ERROR],
output: 'import f from "../path"',
}),
tInvalid({
code: `import f from "${absolutePath('/some/path')}"`,
filename: absolutePath('/foo/bar/index.js'),
code: `import f from "/some/path"`,
filename: '/foo/bar/index.js',
errors: [ABSOLUTE_ERROR],
output: 'import f from "../../some/path"',
}),
tInvalid({
code: `import f from "${absolutePath('/some/path')}"`,
filename: absolutePath('/foo/bar/index.js'),
code: `import f from "/some/path"`,
filename: '/foo/bar/index.js',
options: [{ amd: true }],
errors: [ABSOLUTE_ERROR],
output: 'import f from "../../some/path"',
}),
tInvalid({
code: `var f = require("${absolutePath('/foo')}")`,
filename: absolutePath('/foo/bar/index.js'),
code: `var f = require("/foo")`,
filename: '/foo/bar/index.js',
errors: [ABSOLUTE_ERROR],
output: 'var f = require("..")',
}),
tInvalid({
code: `var f = require("${absolutePath('/foo/path')}")`,
filename: absolutePath('/foo/bar/index.js'),
code: `var f = require("/foo/path")`,
filename: '/foo/bar/index.js',
errors: [ABSOLUTE_ERROR],
output: 'var f = require("../path")',
}),
tInvalid({
code: `var f = require("${absolutePath('/some/path')}")`,
filename: absolutePath('/foo/bar/index.js'),
code: `var f = require("/some/path")`,
filename: '/foo/bar/index.js',
errors: [ABSOLUTE_ERROR],
output: 'var f = require("../../some/path")',
}),
tInvalid({
code: `var f = require("${absolutePath('/some/path')}")`,
filename: absolutePath('/foo/bar/index.js'),
code: `var f = require("/some/path")`,
filename: '/foo/bar/index.js',
options: [{ amd: true }],
errors: [ABSOLUTE_ERROR],
output: 'var f = require("../../some/path")',
}),
// validate amd
tInvalid({
code: `require(["${absolutePath('/some/path')}"], function (f) { /* ... */ })`,
filename: absolutePath('/foo/bar/index.js'),
code: `require(["/some/path"], function (f) { /* ... */ })`,
filename: '/foo/bar/index.js',
options: [{ amd: true }],
errors: [ABSOLUTE_ERROR],
output: 'require(["../../some/path"], function (f) { /* ... */ })',
}),
tInvalid({
code: `define(["${absolutePath('/some/path')}"], function (f) { /* ... */ })`,
filename: absolutePath('/foo/bar/index.js'),
code: `define(["/some/path"], function (f) { /* ... */ })`,
filename: '/foo/bar/index.js',
languageOptions: { parser: require(parsers.ESPREE) },
options: [{ amd: true }],
errors: [ABSOLUTE_ERROR],
Expand Down
Loading