docs(linter): Improve docs for import/extensions and add a few more tests#17539
docs(linter): Improve docs for import/extensions and add a few more tests#17539graphite-app[bot] merged 1 commit intomainfrom
import/extensions and add a few more tests#17539Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for Node.js subpath imports (imports starting with #) to the import/extensions linter rule, adds test cases for import attributes, and improves documentation to clarify best practices for the always configuration option.
Key changes:
- Implements detection of subpath imports (e.g.,
#internal/z) by distinguishing them from package imports and path aliases - Adds comprehensive test cases for both pass and fail scenarios with subpath imports
- Enhances documentation with warnings about using
ignorePackages: truewhen setting the rule toalways
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
crates/oxc_linter/src/rules/import/extensions.rs |
Adds subpath import detection logic in is_package_import() function, improves documentation with IMPORTANT notes and examples, adds inline comments explaining #[serde(skip)] attributes, and adds test cases for subpath imports and import attributes |
crates/oxc_linter/src/snapshots/import_extensions.snap |
Adds snapshot test outputs for subpath import test cases showing expected error messages for missing and unwanted extensions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
CodSpeed Performance ReportMerging #17539 will not alter performanceComparing Summary
Footnotes
|
5e2a66a to
23ce152
Compare
import/extensions and improve docsimport/extensions and add a few more tests
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Merge activity
|
… tests (#17539) This PR makes the following changes: - Add a few more test cases to ensure imports with assertions work fine (`import foo from "./foo.json" with { type: "json" }`) - Improves the docs for the `import/extensions` rule to make clear that `always` should be used with the `ignorePackages` setting in almost all cases. - Add tests for subpath import support, but comment them out (`import internalZ from "#internal/z";`). I decided to split this out of the current PR because subpath import support is a bit more complex than I had thought (mainly the case of how we use `#oxlint` in our own code and whether/how we can avoid treating that as a violation). Related to #17501. Docs: ### What it does Some file resolve algorithms allow you to omit the file extension within the import source path. For example the node resolver (which does not yet support ESM/import) can resolve ./foo/bar to the absolute path /User/someone/foo/bar.js because the .js extension is resolved automatically by default in CJS. Depending on the resolver you can configure more extensions to get resolved automatically. In order to provide a consistent use of file extensions across your code base, this rule can enforce or disallow the use of certain file extensions. ### Why is this bad? ESM-based file resolve algorithms (e.g., the one that Vite provides) recommend specifying the file extension to improve performance. ### Examples Examples of **incorrect** code for this rule: The following patterns are considered problems when configuration set to "always": ```js import foo from "./foo"; import bar from "./bar"; import Component from "./Component"; import foo from "@/foo"; ``` The following patterns are considered problems when configuration set to "never": ```js import foo from "./foo.js"; import bar from "./bar.json"; import Component from "./Component.jsx"; import express from "express/index.js"; ``` Examples of **correct** code for this rule: The following patterns are not considered problems when configuration set to "always": ```js import foo from "./foo.js"; import bar from "./bar.json"; import Component from "./Component.jsx"; import * as path from "path"; import foo from "@/foo.js"; ``` The following patterns are not considered problems when configuration set to "never": ```js import foo from "./foo"; import bar from "./bar"; import Component from "./Component"; import express from "express/index"; import * as path from "path"; ``` **Per-extension configuration examples**: ```js // Configuration: { "vue": "always", "ts": "never" } import Component from "./Component.vue"; // ✓ OK - .vue configured as "always" import utils from "./utils"; // ✓ OK - .ts configured as "never" import styles from "./styles.css"; // ✓ OK - .css not configured, ignored // Configuration: ["ignorePackages", { "js": "never", "ts": "never" }] import foo from "./foo"; // ✓ OK - no extension import bar from "lodash/fp"; // ✓ OK - package import, ignored (ignorePackages sets this to true) ``` ## Configuration This rule accepts three types of configuration: 1. **Global rule** (string): `"always"`, `"never"`, or `"ignorePackages"` ```jsonc { "rules": { // this would require extensions for all imports, *including from packages* // e.g. `import React from 'react';` would be disallowed. // You should generally always set `ignorePackages` to `true` when using `always`. "import/extensions": ["error", "always"], }, } ``` 2. **Per-extension rules** (object): `{ "js": "always", "jsx": "never", ... }` ```jsonc { "rules": { "import/extensions": [ "error", // per-extension rules: // require extensions for .js imports and disallow them for .ts imports { "js": "always", "ts": "never", "ignorePackages": true }, ], }, } ``` 3. **Combined** (array): `["error", "always", { "js": "never" }]` or `["error", { "js": "always" }]` ```jsonc { "rules": { "import/extensions": [ "error", "always", // by default, require extensions for all imports { "ts": "never", // override the global value and disallow extensions on imports for specific file types "ignorePackages": true, }, ], }, } ``` **Default behavior (no configuration)**: All imports - of all kinds - pass. Unconfigured file extensions are ignored, to avoid false positives. This rule accepts a configuration object with the following properties: ### checkTypeImports type: `boolean` default: `false` Whether to check type imports when enforcing extension rules. ```ts // If checkTypeImports is `false`, we don't care about // whether these imports have file extensions or not, both are always allowed: import type { Foo } from "./foo"; import type { Foo } from "./foo.ts"; ``` ### ignorePackages type: `boolean` default: `false` Whether to ignore package imports when enforcing extension rules. > [!IMPORTANT] > When setting this rule to `always`, you should also set `ignorePackages` to `true`. > Otherwise, package imports without extensions (such as `import React from 'react';`) > will be disallowed, which is not desirable and is not fixable. A boolean option (not per-extension) that exempts package imports from the "always" rule. Can be set in the config object: `["error", "always", { "ignorePackages": true }]` Legacy shorthand: `["error", "ignorePackages"]` is equivalent to `["error", "always", { "ignorePackages": true }]` - **With "always"**: When `true`, package imports (e.g., `lodash`, `@babel/core`) don't require extensions - **With "never"**: This option has no effect; extensions are still forbidden on package imports Example: `["error", "always", { "ignorePackages": true }]` allows `import foo from "lodash"` but requires `import bar from "./bar.js"` ### pathGroupOverrides type: `array` default: `[]` Path group overrides for bespoke import specifiers. Array of pattern-action pairs for custom import protocols (monorepo tools, custom resolvers). Each override has: `{ "pattern": "<glob-pattern>", "action": "enforce" | "ignore" }` **Pattern matching**: Uses glob patterns (`*`, `**`, `{a,b}`) to match import specifiers. Note that the pattern matching is done in Rust with the fast-glob library, and so may differ from the JavaScript glob library used by the original ESLint rule. **Actions**: - `"enforce"`: Apply normal extension validation (respect global/per-extension rules) - `"ignore"`: Skip all extension validation for matching imports **Precedence**: First matching pattern wins. **Examples:** ```json { "pattern": "rootverse{*,*/**}", "action": "ignore" } ``` Matches imports from `rootverse+debug:src`, `rootverse+bfe:src/symbols` and ignores whether or not they have an extension. #### pathGroupOverrides[n] type: `object` ##### pathGroupOverrides[n].action type: `"enforce" | "ignore"` Action to take for path group overrides. Determines how import extensions are validated for matching bespoke import specifiers. ###### `"enforce"` Enforce extension validation for matching imports (require extensions based on config). ###### `"ignore"` Ignore matching imports entirely (skip all extension validation). ##### pathGroupOverrides[n].pattern type: `string` Glob pattern to match import specifiers. This uses Rust's fast-glob library for matching.
c39e533 to
655afc1
Compare
# Oxlint ### 💥 BREAKING CHANGES - f7da875 oxlint: [**BREAKING**] Remove oxc_language_server binary (#17457) (Boshen) ### 🚀 Features - 659c23e linter: Init note field boilerplate (#17589) (Shrey Sudhir) - 6870b64 parser: Add TS1363 error code (#17609) (Sysix) - 6154c8c linter/eslint-plugin-vitest: Implemented vitest/warn-todo rule (#17228) (Said Atrahouch) - 0043cd6 linter/eslint-plugin-vitest: Implement consistent-vitest-vi rule (#17389) (Said Atrahouch) - a6d773d linter: Add full TS support to eslint/no-useless-constructor (#17592) (camc314) - f02c0e7 linter/eslint: Implement complexity (#17569) (Nguyen Tran) - bc7aae7 linter/no-unused-vars: Add fixer to remove unused catch bindings (#17567) (Don Isaac) - 9e8ec78 linter/only-throw-error rule: Add `allowRethrowing` option for (#17554) (camc314) - b67e819 linter: Add fixer for `unicorn/prefer-response-static-json` rule (#17559) (Mikhail Baev) - 44b0361 linter/vue: Implement no-this-in-before-route-enter (#17525) (yefan) - ee34716 linter/react: Implement no-will-update-set-state (#17530) (Kenzo Wada) - 3088e1d linter/react: Implement no-this-in-sfc (#17535) (Kenzo Wada) - 29a2868 linter/jsx-a11y: Implement no-static-element-interactions (#17538) (Kenzo Wada) - eadf057 linter: Enable tsconfig auto discovery by default (#17489) (Boshen) - 12a7d6e website_linter: Add a count of rules with fixes available to rules table. (#17476) (Connor Shea) ### 🐛 Bug Fixes - a702f13 oxlint/lsp: Correct position for "disable for this file" with shebang (#17613) (Sysix) - 19fdfb6 linter: Panic in `sort-keys` rule with Unicode numeric characters (#17629) (Adel Rodríguez) - 2e8f469 vscode: Search for `node_modules/.bin/oxlint.exe` too (bun setup) (#17597) (Sysix) - be39906 linter/aria-proptypes: Allow template literals with expressions for string-type ARIA props (#17460) (Jökull Sólberg Auðunsson) - 529901c linter: Include JS plugin rules when calculating total rule count (#17520) (connorshea) - 96ef2cc linter: Print total rule # when using a single nested config (#17517) (connorshea) - 9ad0f29 oxlint: Do not enable external plugin store when no external linter is passed (#17498) (Sysix) - 174375d oxfmt,oxlint: Disable mimalloc for 32-bit Arm targets (#17473) (Yaksh Bariya) - ff70fe9 linter/no-standalone-expect: Allows expect in wrapper functions passed to test blocks (#17427) (Copilot) - dab232f linter/catch-or-return: Handle arrow functions with implicit returns correctly (#17440) (Copilot) - a38892a linter: Update no-unnecessary-template-expression docs and test case (#17453) (camc314) ### ⚡ Performance - 605dbf1 vscode: Restrict searching for oxlint/oxfmt binaries only 3 levels deep + 10s timeout (#17345) (Sysix) ### 📚 Documentation - 884fb63 linter/react: Improve docs for jsx-curly-brace-presence (#17579) (connorshea) - 1d3ee07 linter: Improve rule explanation for `vue/no-this-in-before-route-enter`. (#17581) (connorshea) - 5f189f8 linter/arrow-body-style: Correctly document default mode option (#17566) (Rägnar O'ock) - bb2e8e4 linter: Add a note to the `typescript/no-var-requires` rule about the missing `allow` option (#17551) (connorshea) - 655afc1 linter: Improve docs for `import/extensions` and add a few more tests (#17539) (connorshea) - 7e5fc90 linter: Update list of plugins that are reserved. (#17516) (connorshea) # Oxfmt ### 💥 BREAKING CHANGES - f7da875 oxlint: [**BREAKING**] Remove oxc_language_server binary (#17457) (Boshen) ### 🚀 Features - 8fd4ea9 oxfmt: `options.embeddedLanguageFormatting` is now `"auto"` by default (#17649) (leaysgur) ### 🐛 Bug Fixes - c9b5d7d formatter/sort_imports: Handle alignable_comment correctly (#17646) (leaysgur) - 453222d formatter: Missing comment handling for end-of-line comments in member chains (#17659) (Dunqing) - 0805ff2 formatter: Incorrect inline comment placement in try-catch (#17657) (Dunqing) - 3a0c782 formatter: Don't move comments into optional call parentheses (#17582) (magic-akari) - 174375d oxfmt,oxlint: Disable mimalloc for 32-bit Arm targets (#17473) (Yaksh Bariya) ### ⚡ Performance - abb28dc oxfmt: Turn of pretty print from sort-package-json (#17452) (Boshen)
|
I think this PR causes huge regressions on my end, I have this config: and now running oxlint is failing on all my files that have local any suggestions? It was working fine with previous version, so I assume this is the PR causing the regression? @connorshea |
|
@ghiscoding you're right, the behavior did change for 1.36.0 vs. 1.37.0 🤔 I don't think this PR should've done anything, let me investigate... |
|
Reverting this and running the rule didn't have any effect, so I'm fairly confident it wasn't this PR. I'm not really sure what changed 🤔 |
|
good to know, FYI the previous version is all good as you can see in my Renovate PR below it didn't bump to latest versions yet because I have a 2 days minimum release age, but I assume it will fail when it bumps to latest in 2 days |
|
Okay, I figured out the cause, although I'm not entirely sure why this would impact things exactly. I reverted eadf057 in my local version of oxlint, and that fixes the problem for your repo. I assume because of your repo having so many tsconfig files everywhere, or something? PR that made that change is #17489 Could you open a new issue about this regression? :) |
okie dokie, here's the issue #17693 |
This PR makes the following changes:
import foo from "./foo.json" with { type: "json" })import/extensionsrule to make clear thatalwaysshould be used with theignorePackagessetting in almost all cases.import internalZ from "#internal/z";). I decided to split this out of the current PR because subpath import support is a bit more complex than I had thought (mainly the case of how we use#oxlintin our own code and whether/how we can avoid treating that as a violation).Related to #17501.
Docs:
What it does
Some file resolve algorithms allow you to omit the file extension within the import source path.
For example the node resolver (which does not yet support ESM/import) can resolve ./foo/bar to the absolute path /User/someone/foo/bar.js because the .js extension is resolved automatically by default in CJS.
Depending on the resolver you can configure more extensions to get resolved automatically.
In order to provide a consistent use of file extensions across your code base, this rule can enforce or disallow the use of certain file extensions.
Why is this bad?
ESM-based file resolve algorithms (e.g., the one that Vite provides) recommend specifying the file extension to improve performance.
Examples
Examples of incorrect code for this rule:
The following patterns are considered problems when configuration set to "always":
The following patterns are considered problems when configuration set to "never":
Examples of correct code for this rule:
The following patterns are not considered problems when configuration set to "always":
The following patterns are not considered problems when configuration set to "never":
Per-extension configuration examples:
Configuration
This rule accepts three types of configuration:
"always","never", or"ignorePackages"{ "rules": { // this would require extensions for all imports, *including from packages* // e.g. `import React from 'react';` would be disallowed. // You should generally always set `ignorePackages` to `true` when using `always`. "import/extensions": ["error", "always"], }, }{ "js": "always", "jsx": "never", ... }{ "rules": { "import/extensions": [ "error", // per-extension rules: // require extensions for .js imports and disallow them for .ts imports { "js": "always", "ts": "never", "ignorePackages": true }, ], }, }["error", "always", { "js": "never" }]or["error", { "js": "always" }]{ "rules": { "import/extensions": [ "error", "always", // by default, require extensions for all imports { "ts": "never", // override the global value and disallow extensions on imports for specific file types "ignorePackages": true, }, ], }, }Default behavior (no configuration): All imports - of all kinds - pass.
Unconfigured file extensions are ignored, to avoid false positives.
This rule accepts a configuration object with the following properties:
checkTypeImports
type:
booleandefault:
falseWhether to check type imports when enforcing extension rules.
ignorePackages
type:
booleandefault:
falseWhether to ignore package imports when enforcing extension rules.
Important
When setting this rule to
always, you should also setignorePackagestotrue.Otherwise, package imports without extensions (such as
import React from 'react';)will be disallowed, which is not desirable and is not fixable.
A boolean option (not per-extension) that exempts package imports from the "always" rule.
Can be set in the config object:
["error", "always", { "ignorePackages": true }]Legacy shorthand:
["error", "ignorePackages"]is equivalent to["error", "always", { "ignorePackages": true }]true, package imports (e.g.,lodash,@babel/core) don't require extensionsExample:
["error", "always", { "ignorePackages": true }]allowsimport foo from "lodash"but requiresimport bar from "./bar.js"pathGroupOverrides
type:
arraydefault:
[]Path group overrides for bespoke import specifiers.
Array of pattern-action pairs for custom import protocols (monorepo tools, custom resolvers).
Each override has:
{ "pattern": "<glob-pattern>", "action": "enforce" | "ignore" }Pattern matching: Uses glob patterns (
*,**,{a,b}) to match import specifiers.Note that the pattern matching is done in Rust with the fast-glob library, and so may differ
from the JavaScript glob library used by the original ESLint rule.
Actions:
"enforce": Apply normal extension validation (respect global/per-extension rules)"ignore": Skip all extension validation for matching importsPrecedence: First matching pattern wins.
Examples:
{ "pattern": "rootverse{*,*/**}", "action": "ignore" }Matches imports from
rootverse+debug:src,rootverse+bfe:src/symbolsandignores whether or not they have an extension.
pathGroupOverrides[n]
type:
objectpathGroupOverrides[n].action
type:
"enforce" | "ignore"Action to take for path group overrides.
Determines how import extensions are validated for matching bespoke import specifiers.
"enforce"Enforce extension validation for matching imports (require extensions based on config).
"ignore"Ignore matching imports entirely (skip all extension validation).
pathGroupOverrides[n].pattern
type:
stringGlob pattern to match import specifiers. This uses Rust's fast-glob library for matching.