From 4c7d762d02f4c03a2f06df47228c6a4e6b6dceda Mon Sep 17 00:00:00 2001 From: toby Date: Thu, 10 Feb 2022 01:52:08 +1100 Subject: [PATCH 1/2] Add support for namespaces and fix imports --- .../__tests__/missing-make-observable.js | 139 +++++++++++++++--- .../src/missing-make-observable.js | 47 +++++- packages/eslint-plugin-mobx/src/utils.js | 17 ++- 3 files changed, 174 insertions(+), 29 deletions(-) diff --git a/packages/eslint-plugin-mobx/__tests__/missing-make-observable.js b/packages/eslint-plugin-mobx/__tests__/missing-make-observable.js index fe3abacd6..83b5bbaac 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,95 @@ 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} +} +`, + } +)) tester.run("missing-make-observable", rule, { valid: [ @@ -172,5 +261,9 @@ tester.run("missing-make-observable", rule, { ...invalid3, ...invalid4, ...invalid5, + ...invalid6, + ...invalid7, + ...invalid8, + ...invalid9, ], }); \ 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..15a4629f4 100644 --- a/packages/eslint-plugin-mobx/src/missing-make-observable.js +++ b/packages/eslint-plugin-mobx/src/missing-make-observable.js @@ -1,13 +1,35 @@ '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 makeObserverImportName = 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; + } + + if (specifier.type === 'ImportSpecifier') { + lastSpecifierImport = specifier; + if (specifier.imported.name === 'makeObservable') { + makeObserverImportName = 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 > [] @@ -28,19 +50,34 @@ function create(context) { } } else { const fix = fixer => { + + const fixes = []; + let makeObservableExpr = 'makeObservable'; + + // Insert the makeObservable import if required + if (!namespaceImportName && !makeObserverImportName && lastSpecifierImport) { + fixes.push(fixer.insertTextAfter(lastSpecifierImport, ', makeObservable')); + } else if (namespaceImportName) { + makeObservableExpr = `${namespaceImportName}.makeObservable`; + } else if (makeObserverImportName) { + makeObservableExpr = makeObserverImportName; + } + 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);') + fixes.push(fixer.insertTextBefore(closingBracket, `;${makeObservableExpr}(this);`)); } else { // 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({ diff --git a/packages/eslint-plugin-mobx/src/utils.js b/packages/eslint-plugin-mobx/src/utils.js index ffc23ef68..804504a1e 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 From 0fe7e04a8045fa19f25bb2de4022559c9c87d892 Mon Sep 17 00:00:00 2001 From: toby Date: Thu, 10 Feb 2022 02:23:56 +1100 Subject: [PATCH 2/2] super --- .../__tests__/missing-make-observable.js | 51 +++++++++++++++++++ .../src/missing-make-observable.js | 49 +++++++++++++----- packages/eslint-plugin-mobx/src/utils.js | 2 +- 3 files changed, 89 insertions(+), 13 deletions(-) diff --git a/packages/eslint-plugin-mobx/__tests__/missing-make-observable.js b/packages/eslint-plugin-mobx/__tests__/missing-make-observable.js index 83b5bbaac..819f929d1 100644 --- a/packages/eslint-plugin-mobx/__tests__/missing-make-observable.js +++ b/packages/eslint-plugin-mobx/__tests__/missing-make-observable.js @@ -248,6 +248,54 @@ constructor() { makeBanana(this); } } )) +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: [ ...valid1, @@ -265,5 +313,8 @@ tester.run("missing-make-observable", rule, { ...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 15a4629f4..a6c698dda 100644 --- a/packages/eslint-plugin-mobx/src/missing-make-observable.js +++ b/packages/eslint-plugin-mobx/src/missing-make-observable.js @@ -6,7 +6,7 @@ function create(context) { const sourceCode = context.getSourceCode(); let namespaceImportName = undefined; // is 'mobxFoo' when import * as mobxFoo from 'mobx' - let makeObserverImportName = undefined; // is 'mobxFoo' when import * as mobxFoo from 'mobx' + let makeObservableExpr = undefined; // is 'mobxFoo' when import * as mobxFoo from 'mobx' let lastSpecifierImport = undefined; return { @@ -18,12 +18,13 @@ function create(context) { 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') { - makeObserverImportName = specifier.local.name; + makeObservableExpr = specifier.local.name; } } } @@ -36,7 +37,9 @@ function create(context) { 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) { @@ -50,17 +53,17 @@ 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 = []; - let makeObservableExpr = 'makeObservable'; // Insert the makeObservable import if required - if (!namespaceImportName && !makeObserverImportName && lastSpecifierImport) { - fixes.push(fixer.insertTextAfter(lastSpecifierImport, ', makeObservable')); - } else if (namespaceImportName) { - makeObservableExpr = `${namespaceImportName}.makeObservable`; - } else if (makeObserverImportName) { - makeObservableExpr = makeObserverImportName; + if (!makeObservableExpr) { + lastSpecifierImport && fixes.push(fixer.insertTextAfter(lastSpecifierImport, ', makeObservable')); + makeObservableExpr = 'makeObservable'; } if (constructor?.value.type === 'TSEmptyBodyFunctionExpression') { @@ -71,7 +74,15 @@ function create(context) { // constructor() {} const closingBracket = sourceCode.getLastToken(constructor.value.body); fixes.push(fixer.insertTextBefore(closingBracket, `;${makeObservableExpr}(this);`)); - } else { + } 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); fixes.push(fixer.insertTextAfter(openingBracket, `\nconstructor() { ${makeObservableExpr}(this); }`)); @@ -82,7 +93,7 @@ function create(context) { context.report({ node: clazz, - messageId: 'missingMakeObservable', + messageId: unsupportedSuper ? 'missingMakeObservableSuper' : 'missingMakeObservable', fix, }) } @@ -90,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', @@ -101,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 804504a1e..24b7ef897 100644 --- a/packages/eslint-plugin-mobx/src/utils.js +++ b/packages/eslint-plugin-mobx/src/utils.js @@ -13,7 +13,7 @@ function isMobxDecorator(decorator, namespace) { memberExpression = decorator.expression.callee; } - if (memberExpression.object.name === namespace || memberExpression.object.object?.name === namespace) { + if (memberExpression?.object.name === namespace || memberExpression?.object.object?.name === namespace) { return true; } }