mobx icon indicating copy to clipboard operation
mobx copied to clipboard

[eslint-plugin-mobx] 'mobx/missing-observer' rule is suggested for all components

Open Aure77 opened this issue 2 years ago • 5 comments

Intended outcome:

only components that use observable should be triggered.

Actual outcome:

mobx/missing-observer rule is suggested for all components even if they doesn't consume observable.

How to reproduce the issue:

Create a React component without observer, ex: Button:

export const Button = ({
  accessible = false,
  children
}: any): JSX.Element => {
   return null; // don't use any observable
}

Add [email protected] then configure eslint with:

// .eslintrc.js
module.exports = {
    parser: "@typescript-eslint/parser",
    plugins: ["mobx"],
    extends: "plugin:mobx/recommended",
}

And run linter. Trigger warning: Component 'Button' is missing 'observer'. eslint(mobx/missing-observer)

Versions

eslint-plugin-mobx: 0.0.9 mobx: 6.9.0 mobx-react: 8.0.0

Aure77 avatar Jul 13 '23 12:07 Aure77

Works as intended according to docs: https://github.com/mobxjs/mobx/tree/main/packages/eslint-plugin-mobx#mobxmissing-observer

There is no reliable way to check whether an observable value is used within a component.

Wrapping all the components with observer can save you time debugging if you forget to wrap a component that reads observable data.

kubk avatar Jul 13 '23 13:07 kubk

Wrapping all the components with observer can save you time debugging if you forget to wrap a component that reads observable data.

Ok, but is it efficient ? (more observer = more components that need to listen state mutation = slower/inefficient ?)

https://mobx.js.org/react-integration.html#always-read-observables-inside-observer-components

The rule of thumb is: apply observer to all components that read observable data.

Any way for eslint to check if a component is importing from "mobx-react" or "mobx-react-lite" ?

Aure77 avatar Jul 13 '23 14:07 Aure77

Any way for eslint to check if a component is importing from "mobx-react" or "mobx-react-lite" ?

Here is my understanding of it. Let's consider the example of a valid component that renders an observable without importing mobx-react or mobx-react-lite:

import React from 'react'
import { counter } from './counter'

export const Counter = () => {
  return <div>{counter.count}</div>
}

And the counter could be anything:

import { makeAutoObservable } from 'mobx'

export const counter = makeAutoObservable({ count: 0 })

And even with Mobx imports the rule should check observable properties:

import { makeAutoObservable } from 'mobx'

class Counter {
  count = 0;

  constructor() {
    makeObservable(this); // note that the `count` isn't observable here, the ESLint rule should take it into account
  }
}

export const counter = new Counter()

An example without Mobx imports:

import { AnotherObservableCounter } from 'another-observable-counter'

class Counter extends AnotherObservableCounter {}

export const counter = new Counter()

Ok, but is it efficient ? (more observer = more components that need to listen state mutation = slower/inefficient ?)

The harm of having many observers is usually lower than the harm of having many non-memoized components. Otherwise, it doesn't make sense to use Mobx: https://github.com/mobxjs/mobx/pull/3219#issuecomment-991961577

kubk avatar Jul 13 '23 15:07 kubk

Ok..

Another downside:

// eslint suggest to add observer here:
const MySimpleComponent = () => {
  ....
};
// but it's already applied at export
export default observer(MySimpleComponent);

Any way to detect only "exported" component if they are not used inside ?

Because this can introduce unwanted error like this: [mobx-react-lite] You are trying to use observer on a function component wrapped in either another observer or React.memo.

Aure77 avatar Jul 17 '23 13:07 Aure77

Probably doable as long as it's in the same file. PR welcome.

Btw there is also a runtime check that is useful for detecting missing observers: https://mobx.js.org/configuration.html#observablerequiresreaction-boolean

urugator avatar Jul 17 '23 13:07 urugator