diff --git a/packages/eslint-plugin-mobx/__tests__/missing-make-observable.js b/packages/eslint-plugin-mobx/__tests__/missing-make-observable.js index fe3abacd6..819f929d1 100644 --- a/packages/eslint-plugin-mobx/__tests__/missing-make-observable.js +++ b/packages/eslint-plugin-mobx/__tests__/missing-make-observable.js @@ -8,22 +8,22 @@ const tester = new RuleTester({ }); const fields = [ - '@observable o = 5', - '@observable.ref or = []', - '@observable.shallow os = []', - '@observable.deep od = {}', - '@computed get c() {}', - '@computed.struct get cs() {}', - '@computed({ equals }) get co() {}', - '@action a() {}', - '@action.bound ab() {}', - '@flow *f() {}', - '@flow.bound *fb() {}', + 'observable o = 5', + 'observable.ref or = []', + 'observable.shallow os = []', + 'observable.deep od = {}', + 'computed get c() {}', + 'computed.struct get cs() {}', + 'computed({ equals }) get co() {}', + 'action a() {}', + 'action.bound ab() {}', + 'flow *f() {}', + 'flow.bound *fb() {}', ]; const valid1 = fields.map(field => ` class C { - ${field} + @${field} constructor() { makeObservable(this) @@ -48,7 +48,7 @@ class C { const valid3 = fields.map(field => ` class C { - ${field} + @${field} constructor() { makeObservable(this, null, { name: 'foo' }) @@ -58,7 +58,7 @@ class C { const valid4 = fields.map(field => ` class C { - ${field} + @${field} constructor(aString: string); constructor(aNum: number); @@ -71,7 +71,7 @@ class C { const invalid1 = fields.map(field => ({ code: ` class C { - ${field} + @${field} } `, errors: [ @@ -80,7 +80,7 @@ class C { output: ` class C { constructor() { makeObservable(this); } - ${field} + @${field} } ` })) @@ -88,7 +88,7 @@ constructor() { makeObservable(this); } const invalid2 = fields.map(field => ({ code: ` class C { - ${field} + @${field} constructor() {} } `, @@ -97,7 +97,7 @@ class C { ], output: ` class C { - ${field} + @${field} constructor() {;makeObservable(this);} } `, @@ -106,7 +106,7 @@ class C { const invalid3 = fields.map(field => ({ code: ` class C { - ${field} + @${field} constructor() { makeObservable({ a: 5 }); } @@ -117,7 +117,7 @@ class C { ], output: ` class C { - ${field} + @${field} constructor() { makeObservable({ a: 5 }); ;makeObservable(this);} @@ -128,7 +128,7 @@ class C { const invalid4 = fields.map(field => ({ code: ` class C { - ${field} + @${field} constructor() } `, @@ -137,7 +137,7 @@ class C { ], output: ` class C { - ${field} + @${field} constructor() { makeObservable(this); } } `, @@ -147,7 +147,7 @@ class C { const invalid5 = fields.map(field => ({ code: ` class C { - ${field} + @${field} constructor() { makeObservable(this, { o: observable.ref }); } @@ -158,6 +158,143 @@ class C { ], })) +const invalid6 = fields.map(field => ({ + code: ` +import * as mobx from 'mobx'; + +class C { + @mobx.${field} +} +`, + errors: [ + { messageId: 'missingMakeObservable' }, + ], + output: ` +import * as mobx from 'mobx'; + +class C { +constructor() { mobx.makeObservable(this); } + @mobx.${field} +} +`, + } +)) + +const invalid7 = fields.map(field => ({ + code: ` +import { foo } from 'mobx'; + +class C { + @${field} +} +`, + errors: [ + { messageId: 'missingMakeObservable' }, + ], + output: ` +import { foo, makeObservable } from 'mobx'; + +class C { +constructor() { makeObservable(this); } + @${field} +} +`, + } +)) + +const invalid8 = fields.map(field => ({ + code: ` +import { foo } from 'mobx'; +import * as mobx from 'mobx'; + +class C { + @mobx.${field} +} +`, + errors: [ + { messageId: 'missingMakeObservable' }, + ], + output: ` +import { foo } from 'mobx'; +import * as mobx from 'mobx'; + +class C { +constructor() { mobx.makeObservable(this); } + @mobx.${field} +} +`, + } +)) + +const invalid9 = fields.map(field => ({ + code: ` +import { makeObservable as makeBanana } from 'mobx'; + +class C { + @${field} +} +`, + errors: [ + { messageId: 'missingMakeObservable' }, + ], + output: ` +import { makeObservable as makeBanana } from 'mobx'; + +class C { +constructor() { makeBanana(this); } + @${field} +} +`, + } +)) + +const invalid10 = fields.map(field => ({ + code: ` +class C extends Component { + @${field} +} +`, + errors: [ + { messageId: 'missingMakeObservable' }, + ], + output: ` +class C extends Component { +constructor(props) { super(props); makeObservable(this); } + @${field} +} +`, + } +)) + +const invalid11 = fields.map(field => ({ + code: ` +class C extends React.Component<{ foo: string }> { + @${field} +} +`, + errors: [ + { messageId: 'missingMakeObservable' }, + ], + output: ` +class C extends React.Component<{ foo: string }> { +constructor(props: { foo: string }) { super(props); makeObservable(this); } + @${field} +} +`, + } +)) + +const invalid12 = fields.map(field => ({ + code: ` +class C extends Banana { + @${field} +} +`, + errors: [ + { messageId: 'missingMakeObservableSuper' }, + ], + } +)) tester.run("missing-make-observable", rule, { valid: [ @@ -172,5 +309,12 @@ tester.run("missing-make-observable", rule, { ...invalid3, ...invalid4, ...invalid5, + ...invalid6, + ...invalid7, + ...invalid8, + ...invalid9, + ...invalid10, + ...invalid11, + ...invalid12, ], }); \ No newline at end of file diff --git a/packages/eslint-plugin-mobx/src/missing-make-observable.js b/packages/eslint-plugin-mobx/src/missing-make-observable.js index 45d40ca78..a6c698dda 100644 --- a/packages/eslint-plugin-mobx/src/missing-make-observable.js +++ b/packages/eslint-plugin-mobx/src/missing-make-observable.js @@ -1,20 +1,45 @@ 'use strict'; -const { findAncestor, isMobxDecorator } = require('./utils.js'); +const {findAncestor, isMobxDecorator} = require('./utils.js'); function create(context) { const sourceCode = context.getSourceCode(); + let namespaceImportName = undefined; // is 'mobxFoo' when import * as mobxFoo from 'mobx' + let makeObservableExpr = undefined; // is 'mobxFoo' when import * as mobxFoo from 'mobx' + let lastSpecifierImport = undefined; + return { + 'ImportDeclaration': node => { + if (node.source.value !== 'mobx') return; + + // Collect the imports + + for (const specifier of node.specifiers) { + if (specifier.type === 'ImportNamespaceSpecifier') { + namespaceImportName = specifier.local.name; + makeObservableExpr = `${namespaceImportName}.makeObservable`; + } + + if (specifier.type === 'ImportSpecifier') { + lastSpecifierImport = specifier; + if (specifier.imported.name === 'makeObservable') { + makeObservableExpr = specifier.local.name; + } + } + } + }, 'Decorator': decorator => { - if (!isMobxDecorator(decorator)) return; + if (!isMobxDecorator(decorator, namespaceImportName)) return; const clazz = findAncestor(decorator, node => node.type === 'ClassDeclaration' || node.type === 'ClassExpression'); if (!clazz) return; // ClassDeclaration > ClassBody > [] const constructor = clazz.body.body.find(node => node.kind === 'constructor' && node.value.type === 'FunctionExpression') ?? clazz.body.body.find(node => node.kind === 'constructor'); // MethodDefinition > FunctionExpression > BlockStatement > [] - const isMakeObservable = node => node.expression?.callee?.name === 'makeObservable' && node.expression?.arguments[0]?.type === 'ThisExpression'; + const isReact = isReactComponent(clazz.superClass); + const unsupportedSuper = !isReact && !!clazz.superClass; + const isMakeObservable = node => (node.expression?.callee?.name === 'makeObservable' || node.expression?.callee?.property?.name === 'makeObservable') && node.expression?.arguments[0]?.type === 'ThisExpression'; const makeObservable = constructor?.value.body?.body.find(isMakeObservable)?.expression; if (makeObservable) { @@ -28,24 +53,47 @@ function create(context) { } } else { const fix = fixer => { + // The class extends a another unknown class so we can not safely create a super call. + if (unsupportedSuper && !constructor) { + return; + } + + const fixes = []; + + // Insert the makeObservable import if required + if (!makeObservableExpr) { + lastSpecifierImport && fixes.push(fixer.insertTextAfter(lastSpecifierImport, ', makeObservable')); + makeObservableExpr = 'makeObservable'; + } + if (constructor?.value.type === 'TSEmptyBodyFunctionExpression') { // constructor() - yes this a thing const closingBracket = sourceCode.getLastToken(constructor.value); - return fixer.insertTextAfter(closingBracket, ' { makeObservable(this); }') + fixes.push(fixer.insertTextAfter(closingBracket, ` { ${makeObservableExpr}(this); }`)); } else if (constructor) { // constructor() {} const closingBracket = sourceCode.getLastToken(constructor.value.body); - return fixer.insertTextBefore(closingBracket, ';makeObservable(this);') - } else { + fixes.push(fixer.insertTextBefore(closingBracket, `;${makeObservableExpr}(this);`)); + } else if (isReact) { + // class C extends Component<{ foo: string }> {} + let propsType = ''; + if (clazz.superTypeParameters?.params.length) { + propsType = `: ${sourceCode.getText(clazz.superTypeParameters.params[0])}`; + } + const openingBracket = sourceCode.getFirstToken(clazz.body); + fixes.push(fixer.insertTextAfter(openingBracket, `\nconstructor(props${propsType}) { super(props); ${makeObservableExpr}(this); }`)); + } else if (!unsupportedSuper) { // class C {} const openingBracket = sourceCode.getFirstToken(clazz.body); - return fixer.insertTextAfter(openingBracket, '\nconstructor() { makeObservable(this); }') + fixes.push(fixer.insertTextAfter(openingBracket, `\nconstructor() { ${makeObservableExpr}(this); }`)); } + + return fixes; }; context.report({ node: clazz, - messageId: 'missingMakeObservable', + messageId: unsupportedSuper ? 'missingMakeObservableSuper' : 'missingMakeObservable', fix, }) } @@ -53,6 +101,19 @@ function create(context) { }; } +function isReactComponent(superClass) { + const classes = ['Component', 'PureComponent']; + if (!superClass) { + return false; + } + + if (classes.includes(superClass.name)) { + return true; + } + + return superClass.object?.name === 'React' && classes.includes(superClass.property.name); +} + module.exports = { meta: { type: 'problem', @@ -64,6 +125,7 @@ module.exports = { }, messages: { missingMakeObservable: "Constructor is missing `makeObservable(this)`.", + missingMakeObservableSuper: "Constructor is missing `makeObservable(this)`. Can not fix because of missing super call.", secondArgMustBeNullish: "`makeObservable`'s second argument must be nullish or not provided when using decorators." }, }, diff --git a/packages/eslint-plugin-mobx/src/utils.js b/packages/eslint-plugin-mobx/src/utils.js index ffc23ef68..24b7ef897 100644 --- a/packages/eslint-plugin-mobx/src/utils.js +++ b/packages/eslint-plugin-mobx/src/utils.js @@ -2,7 +2,22 @@ const mobxDecorators = new Set(['observable', 'computed', 'action', 'flow', 'override']); -function isMobxDecorator(decorator) { +function isMobxDecorator(decorator, namespace) { + if (namespace !== undefined) { + let memberExpression; + if (decorator.expression.type === 'MemberExpression') { + memberExpression = decorator.expression; + } + + if (decorator.expression.type === 'CallExpression' && decorator.expression.callee.type === 'MemberExpression') { + memberExpression = decorator.expression.callee; + } + + if (memberExpression?.object.name === namespace || memberExpression?.object.object?.name === namespace) { + return true; + } + } + return mobxDecorators.has(decorator.expression.name) // @foo || mobxDecorators.has(decorator.expression.callee?.name) // @foo() || mobxDecorators.has(decorator.expression.object?.name) // @foo.bar