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

No mechanism to keep child React components MobX-agnostic when using custom atoms #3770

Open
geomaster opened this issue Oct 10, 2023 · 4 comments
Labels

Comments

@geomaster
Copy link

geomaster commented Oct 10, 2023

When using regular observables, the documentation suggests that toJS() at the observer level can be used to keep child components MobX-agnostic.

However, when using custom atoms toJS() has no effect, as tracking is decoupled from the data itself.

This leaves us with no way to apply MobX gradually to a complex component tree with minimal pain when using atoms. The suggested way of simply applying observer to all child components is very disruptive, since components need to be deeply refactored to dereference data in the proper way, and common components lose their generality as they become coupled to the shape of the data.

Here's one of a few illustrative examples we have encountered when following the approach of decorating all child components:

const Parent = observer(() => {
    const store = useSomeStore();
    const items = store.getItems(); // a plain JS array
    return <Child items={items} />;
});

const Child = observer((props: { items: Item[] }) => {
    // item.name is a getter that calls `reportObserved()` on an associated atom
    return props.items.map(item => (<p>{item.name}</p>));
});

This works fine when the items array is non-empty. However, if it's empty, we get:

Derivation observer is created/updated without reading any observable value.

This makes sense, because the <Child> component never actually calls reportObserved() on any atom. However, to solve this, we must either resort to:

  • moving the store.getItems() logic to the child component, impacting the generality of our components, since <Child> is often a common component used from many different contexts, or
  • converting the items prop to getItems which <Child> would call, which then requires the use of useCallback() and can propagate arbitrarily up the hierarchy, causing inconsistency in components and requiring a large engineering effort to implement.

Is there any guidance on how to have a top-level component become an observer and not requiring large code changes to the complex component tree underneath in the presence of custom atoms? Perhaps a way to make observer consider the component and its descendants as a whole? Are we understanding or using atoms incorrectly?

PS. We love MobX, heartfelt thank you to the team for building and maintaining this amazing piece of tech.

@geomaster geomaster changed the title No mechanism to keep child components MobX-agnostic when using custom atoms No mechanism to keep child React components MobX-agnostic when using custom atoms Oct 10, 2023
@mweststrate
Copy link
Member

Some thoughts:

  1. Derivation observer is created/updated without reading any observable value.. That is not a default warning, it is a warning that can be customly enabled with mobx.configure, so you might want to consider just disabling it
  2. Using toJS like you do in your example seems an anti pattern, it is only a last resort to send data to a component you don't control and cant make observer. The whole point of MobX is that it gets faster due to localized observable read. toJS defeats that purpose. See https://mobx.js.org/understanding-reactivity.html for why.
  3. toJS is a rather dumb utility, it is quite easy to roll your own that also respects your custom atoms. (but again, see 2, this doesn't sound like the correct solution to me)

Is there any guidance on how to have a top-level component become an observer and not requiring large code changes to the complex component tree underneath in the presence of custom atoms? Perhaps a way to make observer consider the component and its descendants as a whole? Are we understanding or using atoms incorrectly?

If you only want observer at the top, you can just as well stick to React.setState, and render your entire tree topdown again on every change. The point of MobX is exactly to not do that, by having every component listen to their own relevant observables, making rendering fast. React doesn't offer an API to automate that, so wrapping observer everywhere is the small price to pay here.

@geomaster
Copy link
Author

geomaster commented Oct 11, 2023

Thanks for the reply @mweststrate!

I didn't realize the warning can be disabled separately from others, that's great news! The only downside is that disabling it would also disable similar warnings when a reaction or autorun is created without reading observables, am I correct in this understanding? I guess it would be a price to pay for going with this approach. Best would be if we could whitelist particular components for which to ignore the warning, but it seems to be a global option.

It's not obvious to me how to roll a version of toJS() which would respect custom atoms. The only approach I can think of is wrapping the data in some kind of a wrapper that could indicate whether or not custom atoms' report...() methods should be triggered. Is this something you had in mind?

If you only want observer at the top, you can just as well stick to React.setState, and render your entire tree topdown again on every change.

We're aware of where our React hotspots are at the moment and know which trees we would ideally decorate with @observer to avoid excessive re-renders. The issue with indiscriminately applying @observer is that the components are not set up to take advantage of this, since they don't follow the late-dereference guideline. But if we refactor them in this manner, their generality will suffer, which will lead to more work and more risk for us.

But disabling this particular warning and applying @observer to all components that touch an atom sounds like the best tradeoff at the moment.

A bit more context (feel free to ignore): We have a large complex piece of state that is independent of the frontend (it's a core data structure also used by the server and in various other places), which we currently represent in MobX using a single 'dummy' observable.ref that we update whenever anything in the data structure changes. This obviously already causes the worst-case "re-render everything" behavior.

We were drawn to the atom system because this data structure already has its own diffing and change tracking, so it was comparably easy to hook up in order to bridge it to the MobX world.

But for the UI code that reads this data structure, there was no forcing function that would have enforced "proper" MobX style, since everything that touched it re-rendered all of the time anyway. For this reason not all components are written in a way that would take full advantage of @observer.

@urugator
Copy link
Collaborator

The only downside is that disabling it would also disable similar warnings when a reaction or autorun is created without reading observables, am I correct in this understanding?

It can be configured per reaction/autorun

requiresObservable?: boolean

Seems missing in docs, PR welcome :)

Technically it's possible to configure it per observer, but currently there is no way to pass the option.

@urugator
Copy link
Collaborator

urugator commented Nov 24, 2023

Workaround mayhaps? :D

const atom = createAtom("dummy");

function suppressRequiresObservableWarning() {
  atom.reportObserved();
}

const Child = observer((props: { items: Item[] }) => {
    suppressRequiresObservableWarning();
    return props.items.map(item => (<p>{item.name}</p>));
});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants