Skip to content

Conversation

@unrevised6419
Copy link

@unrevised6419 unrevised6419 commented Nov 27, 2025

Overview

When having the ESLint config in a *.ts file, we have a TS error Could not find a declaration file for module './.wxt/eslint-auto-imports.mjs'

Before After
image image

Manual Testing

  1. Have ESLint 9 in your project
  2. have ESLint config be in a TS file
  3. Use WXT autoImports ESLint Plugin

@netlify
Copy link

netlify bot commented Nov 27, 2025

Deploy Preview for creative-fairy-df92c4 ready!

Name Link
🔨 Latest commit bd7d30e
🔍 Latest deploy log https://app.netlify.com/projects/creative-fairy-df92c4/deploys/6943756acdc75a0008cda6ba
😎 Deploy Preview https://deploy-preview-1981--creative-fairy-df92c4.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@unrevised6419 unrevised6419 changed the title fix(imports): generate d.ts file for eslint auto imports fix(imports): generate d.mts file for eslint auto imports Nov 27, 2025
@unrevised6419 unrevised6419 force-pushed the fix/eslint-auto-import-types branch 8 times, most recently from 30810fa to 0710f52 Compare November 28, 2025 08:15
@unrevised6419 unrevised6419 changed the title fix(imports): generate d.mts file for eslint auto imports feat(imports): allow eslint >9, generate d.mts file for eslint auto imports Nov 28, 2025
@unrevised6419 unrevised6419 changed the title feat(imports): allow eslint >9, generate d.mts file for eslint auto imports fix(imports): generate d.mts file for eslint auto imports Nov 28, 2025
@unrevised6419 unrevised6419 marked this pull request as draft December 3, 2025 18:53
@unrevised6419 unrevised6419 force-pushed the fix/eslint-auto-import-types branch from 747ee15 to d4f8d98 Compare December 6, 2025 20:54
@unrevised6419 unrevised6419 marked this pull request as ready for review December 6, 2025 20:56
Copy link
Member

@aklinker1 aklinker1 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I like the addition of a declaration file, but there are lots of extra changes here that are not needed, and some that make this a breaking change:

  1. I would prefer to keep the "8" and "9" labeling over "legacy". I expect ESLint will make more breaking changes to their config in future and using version numbers will be better for identifying versions in the long term. I don't see a reason to rename change change the functions related to this.

  2. Making changes to the ResolvedConfig type is a breaking change. WXT Modules depend on this type, so renaming or changing properties is a breaking change. I'm fine with making breaking changes in general, but they need to be in isolated, small PRs into the major branch.

    That being said, in this case, I don't think the rename provides much value, not enough to warrant a breaking change in the first place, so I'm recommending we just not make this change.

* Default:
* - 9 and above: './.wxt/types/wxt-auto-imports.d.mts'
*/
definitionPath?: string;
Copy link
Member

@aklinker1 aklinker1 Dec 16, 2025

Choose a reason for hiding this comment

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

.d.ts files are "declaration" files, not "definition" files.

Suggested change
definitionPath?: string;
declarationFilePath?: string;

Copy link
Member

Choose a reason for hiding this comment

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

Actually, do we even need this config? It should always be the same as the JS file, right? Just with a different extension.

Copy link
Author

Choose a reason for hiding this comment

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

I would prefer to keep the "8" and "9" labeling over "legacy"

Will revert to 8 & 9, but not sure if this will work with upcoming eslint@10

Making changes to the ResolvedConfig type is a breaking change.

Got it, will revert. I thought is only internal.

Copy link
Author

Choose a reason for hiding this comment

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

Actually, do we even need this config?

maybe not, but each type of js file needs its own counterpart of d.ts file

.js -> .d.ts
.mjs -> .d.mts
.cjs -> .d.cts

Copy link
Author

Choose a reason for hiding this comment

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

Probably we can have an internal mapper, and based on the extension to chose ts,cts,mts

Copy link
Author

@unrevised6419 unrevised6419 Dec 17, 2025

Choose a reason for hiding this comment

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

Question: When user defines filePath does it have some restrictions (relative or absolute) regarding the .wxt directory? Because I see in the code it always tries to resolve to the .wxt directory.

Copy link
Author

@unrevised6419 unrevised6419 Dec 17, 2025

Choose a reason for hiding this comment

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

Because getUnimportEslintOptions will resolve it to .wxt directory, and later after returning the resovled config from getUnimportOptions, the defu overrides it again with the user value. It has strange behaviour.

Copy link
Author

Choose a reason for hiding this comment

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

For example

filePath: '.wxt/eslint-auto-imports.cjs',
image

Copy link
Author

@unrevised6419 unrevised6419 Dec 17, 2025

Choose a reason for hiding this comment

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

From my understanding the user config file path needs to be a resolved path, because the default value is a resolved one.

Copy link
Member

Choose a reason for hiding this comment

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

The intention is to allow the user to use either a relative or absolute path in the config. The relative path should be relative to the .wxt directory. In the final resolved config passed around internally, the path should be absolute.

So it looks like there's a bug in resolve-config module responsible for this. You can see how we resolve paths manually outside of defu for all the other major file paths/directories:

const srcDir = path.resolve(root, mergedConfig.srcDir ?? root);

Shouldn't effect this PR, I'd prefer we fix it in a separate PR. I can do it unless you want to.

@unrevised6419 unrevised6419 force-pushed the fix/eslint-auto-import-types branch 5 times, most recently from cd7edec to 737873c Compare December 17, 2025 15:37
@unrevised6419 unrevised6419 changed the title fix(imports): generate d.mts file for eslint auto imports fix(imports): generate d.ts file for eslint>9 auto imports Dec 17, 2025
@unrevised6419 unrevised6419 changed the title fix(imports): generate d.ts file for eslint>9 auto imports fix(imports): generate d.ts file for eslint gte 9 auto imports Dec 17, 2025
@unrevised6419 unrevised6419 force-pushed the fix/eslint-auto-import-types branch from 737873c to b4d1f8c Compare December 17, 2025 16:10
Copy link
Author

Choose a reason for hiding this comment

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

Note: Netlify failed because this file was not in sync with package.json

Copy link
Member

Choose a reason for hiding this comment

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

The rest of the checks will fail as well. You'll need to run:

git fetch
git checkout origin/main pnpm-lock.yaml
pnpm i

to reset it.

Copy link
Member

Choose a reason for hiding this comment

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

Oh shoot sorry, main is broken!

Copy link
Member

@aklinker1 aklinker1 left a comment

Choose a reason for hiding this comment

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

Nice, I like the new changes. Let's just add tests then the PR is good to go.

Copy link
Member

Choose a reason for hiding this comment

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

The rest of the checks will fail as well. You'll need to run:

git fetch
git checkout origin/main pnpm-lock.yaml
pnpm i

to reset it.

Comment on lines +60 to +65
imports: {
eslintrc: {
enabled: true,
filePath: path.resolve('.wxt/eslintrc-auto-import.json'),
},
},
Copy link
Member

Choose a reason for hiding this comment

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

Instead of changing the demo project, please write tests to test different config. ESLint tests are here:

describe('eslintrc', () => {

* Default:
* - 9 and above: './.wxt/types/wxt-auto-imports.d.mts'
*/
definitionPath?: string;
Copy link
Member

Choose a reason for hiding this comment

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

The intention is to allow the user to use either a relative or absolute path in the config. The relative path should be relative to the .wxt directory. In the final resolved config passed around internally, the path should be absolute.

So it looks like there's a bug in resolve-config module responsible for this. You can see how we resolve paths manually outside of defu for all the other major file paths/directories:

const srcDir = path.resolve(root, mergedConfig.srcDir ?? root);

Shouldn't effect this PR, I'd prefer we fix it in a separate PR. I can do it unless you want to.

Comment on lines +152 to +162
const javaScriptFileDirname = path.dirname(options.eslintrc.filePath);
const javaScriptFileExtension = path.extname(options.eslintrc.filePath);
const javaScriptFileBasename = path.basename(
options.eslintrc.filePath,
javaScriptFileExtension,
);

const typeScriptFilePath = path.join(
javaScriptFileDirname,
`${javaScriptFileBasename}.d.ts`,
);
Copy link
Member

@aklinker1 aklinker1 Dec 18, 2025

Choose a reason for hiding this comment

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

This could be simplified a little bit

Suggested change
const javaScriptFileDirname = path.dirname(options.eslintrc.filePath);
const javaScriptFileExtension = path.extname(options.eslintrc.filePath);
const javaScriptFileBasename = path.basename(
options.eslintrc.filePath,
javaScriptFileExtension,
);
const typeScriptFilePath = path.join(
javaScriptFileDirname,
`${javaScriptFileBasename}.d.ts`,
);
const typeScriptFilePath = options.eslintrc.filePath.slice(0, -extname(options.eslintrc.filePath)) + ".d.ts"

The second slice argument might be off by one, I can never remember if extname returns the extension with or without the dot. Or just remove the first dot from ".d.ts".

@codecov
Copy link

codecov bot commented Dec 18, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.17%. Comparing base (43f6c06) to head (bd7d30e).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1981      +/-   ##
==========================================
+ Coverage   75.99%   76.17%   +0.18%     
==========================================
  Files         113      113              
  Lines        3049     3056       +7     
  Branches      686      686              
==========================================
+ Hits         2317     2328      +11     
+ Misses        649      644       -5     
- Partials       83       84       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Dec 18, 2025

Open in StackBlitz

@wxt-dev/analytics

npm i https://pkg.pr.new/@wxt-dev/analytics@1981

@wxt-dev/auto-icons

npm i https://pkg.pr.new/@wxt-dev/auto-icons@1981

@wxt-dev/browser

npm i https://pkg.pr.new/@wxt-dev/browser@1981

@wxt-dev/i18n

npm i https://pkg.pr.new/@wxt-dev/i18n@1981

@wxt-dev/module-react

npm i https://pkg.pr.new/@wxt-dev/module-react@1981

@wxt-dev/module-solid

npm i https://pkg.pr.new/@wxt-dev/module-solid@1981

@wxt-dev/module-svelte

npm i https://pkg.pr.new/@wxt-dev/module-svelte@1981

@wxt-dev/module-vue

npm i https://pkg.pr.new/@wxt-dev/module-vue@1981

@wxt-dev/runner

npm i https://pkg.pr.new/@wxt-dev/runner@1981

@wxt-dev/storage

npm i https://pkg.pr.new/@wxt-dev/storage@1981

@wxt-dev/unocss

npm i https://pkg.pr.new/@wxt-dev/unocss@1981

@wxt-dev/webextension-polyfill

npm i https://pkg.pr.new/@wxt-dev/webextension-polyfill@1981

wxt

npm i https://pkg.pr.new/wxt@1981

commit: bd7d30e

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.

2 participants