Skip to content
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

feat(eslint-plugin-mobx): configurable default autofix annotation for makeObservable #3881

Merged
merged 4 commits into from
May 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/lovely-lemons-joke.md
Original file line number Diff line number Diff line change
@@ -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.
16 changes: 14 additions & 2 deletions packages/eslint-plugin-mobx/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,22 @@ module.exports = {
### mobx/exhaustive-make-observable

Makes sure that `makeObservable` annotates all fields defined on class or object literal.<br>
**Autofix** adds `field: true` for each missing field.<br>
To exclude a field, annotate it using `field: false`.<br>
Does not support fields introduced by constructor (`this.foo = 5`).<br>
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...).<br>
**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

Expand Down
180 changes: 100 additions & 80 deletions packages/eslint-plugin-mobx/src/exhaustive-make-observable.js
Original file line number Diff line number Diff line change
@@ -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({
Expand All @@ -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(", ") + ","
urugator marked this conversation as resolved.
Show resolved Hide resolved
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,
};
create
}