-
-
Notifications
You must be signed in to change notification settings - Fork 431
fix(imports): generate d.ts file for eslint gte 9 auto imports #1981
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
base: main
Are you sure you want to change the base?
fix(imports): generate d.ts file for eslint gte 9 auto imports #1981
Conversation
✅ Deploy Preview for creative-fairy-df92c4 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
30810fa to
0710f52
Compare
747ee15 to
d4f8d98
Compare
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.
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:
-
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.
-
Making changes to the
ResolvedConfigtype 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 themajorbranch.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.
packages/wxt/src/types.ts
Outdated
| * Default: | ||
| * - 9 and above: './.wxt/types/wxt-auto-imports.d.mts' | ||
| */ | ||
| definitionPath?: string; |
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.
.d.ts files are "declaration" files, not "definition" files.
| definitionPath?: string; | |
| declarationFilePath?: string; |
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.
Actually, do we even need this config? It should always be the same as the JS file, right? Just with a different extension.
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 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.
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.
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
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.
Probably we can have an internal mapper, and based on the extension to chose ts,cts,mts
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.
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.
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.
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.
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.
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.
From my understanding the user config file path needs to be a resolved path, because the default value is a resolved one.
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.
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.
cd7edec to
737873c
Compare
737873c to
b4d1f8c
Compare
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.
Note: Netlify failed because this file was not in sync with package.json
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.
The rest of the checks will fail as well. You'll need to run:
git fetch
git checkout origin/main pnpm-lock.yaml
pnpm ito reset it.
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.
Oh shoot sorry, main is broken!
aklinker1
left a comment
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.
Nice, I like the new changes. Let's just add tests then the PR is good to go.
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.
The rest of the checks will fail as well. You'll need to run:
git fetch
git checkout origin/main pnpm-lock.yaml
pnpm ito reset it.
| imports: { | ||
| eslintrc: { | ||
| enabled: true, | ||
| filePath: path.resolve('.wxt/eslintrc-auto-import.json'), | ||
| }, | ||
| }, |
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.
Instead of changing the demo project, please write tests to test different config. ESLint tests are here:
| describe('eslintrc', () => { |
packages/wxt/src/types.ts
Outdated
| * Default: | ||
| * - 9 and above: './.wxt/types/wxt-auto-imports.d.mts' | ||
| */ | ||
| definitionPath?: string; |
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.
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.
| 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`, | ||
| ); |
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.
This could be simplified a little bit
| 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 Report✅ All modified and coverable lines are covered by tests. 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. 🚀 New features to boost your workflow:
|
@wxt-dev/analytics
@wxt-dev/auto-icons
@wxt-dev/browser
@wxt-dev/i18n
@wxt-dev/module-react
@wxt-dev/module-solid
@wxt-dev/module-svelte
@wxt-dev/module-vue
@wxt-dev/runner
@wxt-dev/storage
@wxt-dev/unocss
@wxt-dev/webextension-polyfill
wxt
commit: |

Overview
When having the ESLint config in a
*.tsfile, we have a TS errorCould not find a declaration file for module './.wxt/eslint-auto-imports.mjs'Manual Testing
autoImportsESLint Plugin