From 44a5fe07fb95c2ba24d8df19f18b57ee92abb1a9 Mon Sep 17 00:00:00 2001 From: Kade Date: Wed, 29 May 2024 16:19:44 -0400 Subject: [PATCH] feat(eslint-plugin-mobx): configurable default autofix annotation for makeObservable (#3881) --- .changeset/lovely-lemons-joke.md | 7 + packages/eslint-plugin-mobx/README.md | 16 +- .../src/exhaustive-make-observable.js | 180 ++++++++++-------- 3 files changed, 121 insertions(+), 82 deletions(-) create mode 100644 .changeset/lovely-lemons-joke.md diff --git a/.changeset/lovely-lemons-joke.md b/.changeset/lovely-lemons-joke.md new file mode 100644 index 000000000..12f3bb808 --- /dev/null +++ b/.changeset/lovely-lemons-joke.md @@ -0,0 +1,7 @@ +--- +"eslint-plugin-mobx": patch +--- + +Adds an option for the `mobx/exhaustive-make-observable` eslint rule to configure whether fields are annotated with `true` or `false` with the autofixer. + +This option defaults to `true` if not present or an invalid value is received to maintain existing behavior. diff --git a/packages/eslint-plugin-mobx/README.md b/packages/eslint-plugin-mobx/README.md index 0e22fdfb6..0484a3ef5 100644 --- a/packages/eslint-plugin-mobx/README.md +++ b/packages/eslint-plugin-mobx/README.md @@ -35,10 +35,22 @@ module.exports = { ### mobx/exhaustive-make-observable Makes sure that `makeObservable` annotates all fields defined on class or object literal.
-**Autofix** adds `field: true` for each missing field.
To exclude a field, annotate it using `field: false`.
Does not support fields introduced by constructor (`this.foo = 5`).
-Does not warn about annotated non-existing fields (there is a runtime check, but the autofix removing the field could be handy...). +Does not warn about annotated non-existing fields (there is a runtime check, but the autofix removing the field could be handy...).
+**Autofix** adds `field: true` for each missing field by default. You can change this behaviour by specifying options in your eslint config: + +```json +{ + "rules": { + "mobx/exhaustive-make-observable": ["error", { "autofixAnnotation": false }] + } +} +``` + +This is a boolean value that controls if the field is annotated with `true` or `false`. +If you are migrating an existing project using `makeObservable` and do not want this rule to override +your current usage (even if it may be wrong), you should run the autofix with the annotation set to `false` to maintain existing behaviour: `eslint --no-eslintrc --fix --rule='mobx/exhaustive-make-observable: [2, { "autofixAnnotation": false }]' .` ### mobx/missing-make-observable diff --git a/packages/eslint-plugin-mobx/src/exhaustive-make-observable.js b/packages/eslint-plugin-mobx/src/exhaustive-make-observable.js index 2b43bb338..2812e91c5 100644 --- a/packages/eslint-plugin-mobx/src/exhaustive-make-observable.js +++ b/packages/eslint-plugin-mobx/src/exhaustive-make-observable.js @@ -1,55 +1,62 @@ -'use strict'; +"use strict" -const { findAncestor, isMobxDecorator } = require('./utils.js'); +const { findAncestor, isMobxDecorator } = require("./utils.js") // TODO support this.foo = 5; in constructor // TODO? report on field as well function create(context) { - const sourceCode = context.getSourceCode(); + const sourceCode = context.getSourceCode() + const autofixAnnotation = context.options[0]?.autofixAnnotation ?? true - function fieldToKey(field) { - // TODO cache on field? - const key = sourceCode.getText(field.key); - return field.computed ? `[${key}]` : key; - } + function fieldToKey(field) { + // TODO cache on field? + const key = sourceCode.getText(field.key) + return field.computed ? `[${key}]` : key + } - return { - 'CallExpression[callee.name="makeObservable"]': makeObservable => { - // Only interested about makeObservable(this, ...) in constructor or makeObservable({}, ...) - // ClassDeclaration - // ClassBody - // MethodDefinition[kind="constructor"] - // FunctionExpression - // BlockStatement - // ExpressionStatement - // CallExpression[callee.name="makeObservable"] - const [firstArg, secondArg] = makeObservable.arguments; - if (!firstArg) return; - let members; - if (firstArg.type === 'ThisExpression') { - const closestFunction = findAncestor(makeObservable, node => node.type === 'FunctionExpression' || node.type === 'FunctionDeclaration') - if (closestFunction?.parent?.kind !== 'constructor') return; - members = closestFunction.parent.parent.parent.body.body; - } else if (firstArg.type === 'ObjectExpression') { - members = firstArg.properties; - } else { - return; - } + return { + 'CallExpression[callee.name="makeObservable"]': makeObservable => { + // Only interested about makeObservable(this, ...) in constructor or makeObservable({}, ...) + // ClassDeclaration + // ClassBody + // MethodDefinition[kind="constructor"] + // FunctionExpression + // BlockStatement + // ExpressionStatement + // CallExpression[callee.name="makeObservable"] + const [firstArg, secondArg] = makeObservable.arguments + if (!firstArg) return + let members + if (firstArg.type === "ThisExpression") { + const closestFunction = findAncestor( + makeObservable, + node => + node.type === "FunctionExpression" || node.type === "FunctionDeclaration" + ) + if (closestFunction?.parent?.kind !== "constructor") return + members = closestFunction.parent.parent.parent.body.body + } else if (firstArg.type === "ObjectExpression") { + members = firstArg.properties + } else { + return + } - const annotationProps = secondArg?.properties || []; - const nonAnnotatedMembers = []; - let hasAnyDecorator = false; + const annotationProps = secondArg?.properties || [] + const nonAnnotatedMembers = [] + let hasAnyDecorator = false - members.forEach(member => { - if (member.static) return; - if (member.kind === "constructor") return; - //if (member.type !== 'MethodDefinition' && member.type !== 'ClassProperty') return; - hasAnyDecorator = hasAnyDecorator || member.decorators?.some(isMobxDecorator) || false; - if (!annotationProps.some(prop => fieldToKey(prop) === fieldToKey(member))) { // TODO optimize? - nonAnnotatedMembers.push(member); - } - }) - /* + members.forEach(member => { + if (member.static) return + if (member.kind === "constructor") return + //if (member.type !== 'MethodDefinition' && member.type !== 'ClassProperty') return; + hasAnyDecorator = + hasAnyDecorator || member.decorators?.some(isMobxDecorator) || false + if (!annotationProps.some(prop => fieldToKey(prop) === fieldToKey(member))) { + // TODO optimize? + nonAnnotatedMembers.push(member) + } + }) + /* // With decorators, second arg must be null/undefined or not provided if (hasAnyDecorator && secondArg && secondArg.name !== "undefined" && secondArg.value !== null) { context.report({ @@ -66,46 +73,59 @@ function create(context) { } */ - if (!hasAnyDecorator && nonAnnotatedMembers.length) { - // Set avoids reporting twice for setter+getter pair or actual duplicates - const keys = [...new Set(nonAnnotatedMembers.map(fieldToKey))]; - const keyList = keys.map(key => `\`${key}\``).join(', '); + if (!hasAnyDecorator && nonAnnotatedMembers.length) { + // Set avoids reporting twice for setter+getter pair or actual duplicates + const keys = [...new Set(nonAnnotatedMembers.map(fieldToKey))] + const keyList = keys.map(key => `\`${key}\``).join(", ") - const fix = fixer => { - const annotationList = keys.map(key => `${key}: true`).join(', ') + ','; - if (!secondArg) { - return fixer.insertTextAfter(firstArg, `, { ${annotationList} }`); - } else if (secondArg.type !== 'ObjectExpression') { - return fixer.replaceText(secondArg, `{ ${annotationList} }`); - } else { - const openingBracket = sourceCode.getFirstToken(secondArg) - return fixer.insertTextAfter(openingBracket, ` ${annotationList} `); - } - }; + const fix = fixer => { + const annotationList = + keys.map(key => `${key}: ${autofixAnnotation}`).join(", ") + "," + if (!secondArg) { + return fixer.insertTextAfter(firstArg, `, { ${annotationList} }`) + } else if (secondArg.type !== "ObjectExpression") { + return fixer.replaceText(secondArg, `{ ${annotationList} }`) + } else { + const openingBracket = sourceCode.getFirstToken(secondArg) + return fixer.insertTextAfter(openingBracket, ` ${annotationList} `) + } + } - context.report({ - node: makeObservable, - messageId: 'missingAnnotation', - data: { keyList }, - fix, - }) - } - }, - }; + context.report({ + node: makeObservable, + messageId: "missingAnnotation", + data: { keyList }, + fix + }) + } + } + } } module.exports = { - meta: { - type: 'suggestion', - fixable: 'code', - docs: { - description: 'enforce all fields being listen in `makeObservable`', - recommended: true, - suggestion: false, - }, - messages: { - 'missingAnnotation': 'Missing annotation for {{ keyList }}. To exclude a field, use `false` as annotation.', + meta: { + type: "suggestion", + fixable: "code", + schema: [ + { + type: "object", + properties: { + autofixAnnotation: { + type: "boolean" + } + }, + additionalProperties: false + } + ], + docs: { + description: "enforce all fields being listen in `makeObservable`", + recommended: true, + suggestion: false + }, + messages: { + missingAnnotation: + "Missing annotation for {{ keyList }}. To exclude a field, use `false` as annotation." + } }, - }, - create, -}; \ No newline at end of file + create +}