test: migrate to createRuleTestCaseFunction
#184
Conversation
🦋 Changeset detectedLatest commit: 5247ee5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
This comment was marked as outdated.
This comment was marked as outdated.
You can either continue to push to this PR or do subsequent PRs, I am OK with both. If you are going to continue to push to this PR, it would be better to convert this PR to a draft. |
There was a problem hiding this comment.
I discovered a type issue with createRuleTestCaseFunction:
in some scenarios the return type is ValidTestCase rather than InvalidTestCase.
e.g.:
test({
code: "import * as namespace from './malformed.js';",
errors: [{ messageId: 'computedReference' }]
})This was caused by the TTestCase declared inside factory signature rather than in the returned signature. If we move the TTestCase in the returned function, alongside TReturn, autocomplete and type checking won't work like they should.
I haven't discovered this earlier because the returned function was inferring the parameter from RuleTester or from the variable type
(e.g.: const valid: RunTests<typeof rule>['valid'])
Another issue related to the wrong return was that valid case with errors were not reported by typecheck.
See these two examples on the main branch:
eslint-plugin-import-x/test/rules/named.spec.ts
Lines 182 to 185 in 14fc608
eslint-plugin-import-x/test/rules/named.spec.ts
Lines 187 to 191 in 14fc608
You can see that both case are inside a valid array.
A possible solution to the problem
A less complex and more straightforward solution is to make a helper
function returning two functions: one for valid case and one for invalid cases.
const { tValid, tInvalid } = createRuleTestCaseFunctions<typeof rule>();Helper implementation
export function createRuleTestCaseFunctions<
TRule extends RuleModule<string, unknown[]>,
TData extends GetRuleModuleTypes<TRule> = GetRuleModuleTypes<TRule>,
Valid = TSESLintValidTestCase<TData['options']>,
Invalid = TSESLintInvalidTestCase<TData['messageIds'], TData['options']>,
>(): { tValid: (t: Valid) => Valid; tInvalid: (t: Invalid) => Invalid } {
return {
tValid: createRuleTestCase as never,
tInvalid: createRuleTestCase as never,
}
}This way we can be sure that
- the test case is correctly type checked
- autosuggestion works as they should
- return type is constant with the testcase
|
Note I already applied the last proposed changes with the last commit since they required less time than I thought! |
|
Found other two Note Now I'll really stop until your review 😅 |
Actually, I have always been reviewing. As far as I can see there is no issue here. Let's keep moving~ |
I have reviewed all the changes so far, LGTM for now! |
|
Be advised, I just upgraded a few deps and changed a few codes. You might wanna rebase if it is going to introduce conflicts to your side. |
|
There are no conflicts but CI is failing on master. Probably is related to the changes on Those error should be fixed with the new approach I'm using here, I'll try to finish the few rules left and then perform a rebase. |
…-unused-modules`’s `missingExports` option
marcalexiei
force-pushed
the
feat/create-test-function
branch
from
440a917 to
8b6c09f
Compare
|
##### [v4.5.0](https://github.com/un-ts/eslint-plugin-import-x/blob/HEAD/CHANGELOG.md#450) ##### Minor Changes - [#192](un-ts/eslint-plugin-import-x#192) [`fbf639b`](un-ts/eslint-plugin-import-x@fbf639b) Thanks [@SukkaW](https://github.com/SukkaW)! - The PR implements the new resolver design proposed in un-ts/eslint-plugin-import-x#40 (comment) ##### For `eslint-plugin-import-x` users Like the ESLint flat config allows you to use js objects (e.g. import and require) as ESLint plugins, the new `eslint-plugin-import-x` resolver settings allow you to use js objects as custom resolvers through the new setting `import-x/resolver-next`: ```js // eslint.config.js import { createTsResolver } from '#custom-resolver'; const { createOxcResolver } = require('path/to/a/custom/resolver'); const resolverInstance = new ResolverFactory({}); const customResolverObject = { interfaceVersion: 3, name: 'my-custom-eslint-import-resolver', resolve(modPath, sourcePath) { const path = resolverInstance.resolve(modPath, sourcePath); if (path) { return { found: true, path }; } return { found: false, path: null } }; }; module.exports = { settings: { // multiple resolvers 'import-x/resolver-next': [ customResolverObject, createTsResolver(enhancedResolverOptions), createOxcResolver(oxcOptions), ], // single resolver: 'import-x/resolver-next': [createOxcResolver(oxcOptions)] } } ``` The new `import-x/resolver-next` no longer accepts strings as the resolver, thus will not be compatible with the ESLint legacy config (a.k.a. `.eslintrc`). Those who are still using the ESLint legacy config should stick with `import-x/resolver`. In the next major version of `eslint-plugin-import-x` (v5), we will rename the currently existing `import-x/resolver` to `import-x/resolver-legacy` (which allows the existing ESLint legacy config users to use their existing resolver settings), and `import-x/resolver-next` will become the new `import-x/resolver`. When ESLint v9 (the last ESLint version with ESLint legacy config support) reaches EOL in the future, we will remove `import-x/resolver-legacy`. We have also made a few breaking changes to the new resolver API design, so you can't use existing custom resolvers directly with `import-x/resolver-next`: ```js // When migrating to `import-x/resolver-next`, you CAN'T use legacy versions of resolvers directly: module.exports = { settings: { // THIS WON'T WORK, the resolver interface required for `import-x/resolver-next` is different. 'import-x/resolver-next': [ require('eslint-import-resolver-node'), require('eslint-import-resolver-webpack'), require('some-custom-resolver') ]; } } ``` For easier migration, the PR also introduces a compat utility `importXResolverCompat` that you can use in your `eslint.config.js`: ```js // eslint.config.js import eslintPluginImportX, { importXResolverCompat } from 'eslint-plugin-import-x'; // or const eslintPluginImportX = require('eslint-plugin-import-x'); const { importXResolverCompat } = eslintPluginImportX; module.exports = { settings: { // THIS WILL WORK as you have wrapped the previous version of resolvers with the `importXResolverCompat` 'import-x/resolver-next': [ importXResolverCompat(require('eslint-import-resolver-node'), nodeResolveOptions), importXResolverCompat(require('eslint-import-resolver-webpack'), webpackResolveOptions), importXResolverCompat(require('some-custom-resolver'), { option1: true, option2: '' }) ]; } } ``` ##### For custom import resolver developers This is the new API design of the resolver interface: ```ts export interface NewResolver { interfaceVersion: 3; name?: string; // This will be included in the debug log resolve: (modulePath: string, sourceFile: string) => ResolvedResult; } // The `ResultNotFound` (returned when not resolved) is the same, no changes export interface ResultNotFound { found: false; path?: undefined; } // The `ResultFound` (returned resolve result) is also the same, no changes export interface ResultFound { found: true; path: string | null; } export type ResolvedResult = ResultNotFound | ResultFound; ``` You will be able to import `NewResolver` from `eslint-plugin-import-x/types`. The most notable change is that `eslint-plugin-import-x` no longer passes the third argument (`options`) to the `resolve` function. We encourage custom resolvers' authors to consume the options outside the actual `resolve` function implementation. You can export a factory function to accept the options, this factory function will then be called inside the `eslint.config.js` to get the actual resolver: ```js // custom-resolver.js exports.createCustomResolver = (options) => { // The options are consumed outside the `resolve` function. const resolverInstance = new ResolverFactory(options); return { name: 'custom-resolver', interfaceVersion: 3, resolve(mod, source) { const found = resolverInstance.resolve(mod, {}); // Of course, you still have access to the `options` variable here inside // the `resolve` function. That's the power of JavaScript Closures~ } } }; // eslint.config.js const { createCustomResolver } = require('custom-resolver') module.exports = { settings: { 'import-x/resolver-next': [ createCustomResolver(options) ]; } } ``` This allows you to create a reusable resolver instance to improve the performance. With the existing version of the resolver interface, because the options are passed to the `resolver` function, you will have to create a resolver instance every time the `resolve` function is called: ```js module.exports = { interfaceVersion: 2, resolve(mod, source) { // every time the `resolve` function is called, a new instance is created // This is very slow const resolverInstance = ResolverFactory.createResolver({}); const found = resolverInstance.resolve(mod, {}); }, }; ``` With the factory function pattern, you can create a resolver instance beforehand: ```js exports.createCustomResolver = (options) => { // `enhance-resolve` allows you to create a reusable instance: const resolverInstance = ResolverFactory.createResolver({}); const resolverInstance = enhanceResolve.create({}); // `oxc-resolver` also allows you to create a reusable instance: const resolverInstance = new ResolverFactory({}); return { name: "custom-resolver", interfaceVersion: 3, resolve(mod, source) { // the same re-usable instance is shared across `resolve` invocations. // more performant const found = resolverInstance.resolve(mod, {}); }, }; }; ``` ##### Patch Changes - [#184](un-ts/eslint-plugin-import-x#184) [`bc4de89`](un-ts/eslint-plugin-import-x@bc4de89) Thanks [@marcalexiei](https://github.com/marcalexiei)! - fix(no-cycle): improves the type declaration of the rule `no-cycle`’s `maxDepth` option - [#184](un-ts/eslint-plugin-import-x#184) [`bc4de89`](un-ts/eslint-plugin-import-x@bc4de89) Thanks [@marcalexiei](https://github.com/marcalexiei)! - fix(first): improves the type declaration of the rule `first`'s option - [#184](un-ts/eslint-plugin-import-x#184) [`bc4de89`](un-ts/eslint-plugin-import-x@bc4de89) Thanks [@marcalexiei](https://github.com/marcalexiei)! - fix(no-unused-modules): improves the type declaration of the rule `no-unused-modules`’s `missingExports` option - [#184](un-ts/eslint-plugin-import-x#184) [`bc4de89`](un-ts/eslint-plugin-import-x@bc4de89) Thanks [@marcalexiei](https://github.com/marcalexiei)! - fix(no-deprecated): improve error message when no description is available
I migrated the first 10 rules test files.
Aside from a fix on
firstrule where value was missing inOptionstype everything should work like a charm.(I added a changeset but unless someone is extracting the type from the rule export nothing will change on user land)
Do you prefer that I continue to push on this PR or do you prefer that I open subsequent PRs to divide the review effort?