solidjs / solid

A declarative, efficient, and flexible JavaScript library for building user interfaces.
https://solidjs.com
MIT License
32.2k stars 919 forks source link

Mobx triggers component constructor if reactive property changed #2211

Closed westbrma closed 2 months ago

westbrma commented 3 months ago

Describe the bug

When using Mobx with Solid, if you reference a reactive property inside the component function it will rerun that code if the property changes. This can cause a component to endlessly re-render and other negative side effects. In the example below, view the console and you can see its hitting the same code over and over.

Your Example Website or App

https://codesandbox.io/p/sandbox/mobx-external-source-forked-23rpd2?file=%2Findex.js

Steps to Reproduce the Bug or Issue

  1. Create a solid component with a state object that uses makeAutoObservable from mobx.
  2. reference any property in the component "constructor" such as console.log(state.counter)
  3. create a timer that changes the referenced variable such as `setInterval(() => {state.counter++;}, 1000);

Expected behavior

As a user I expect the non JSX code of a component component to only run once. Otherwise it behaves like React where you need to use a hook to prevent the code from running multiple times.

Screenshots or Videos

No response

Platform

Additional context

No response

GrinZero commented 3 months ago

Sandbox link is invalid

westbrma commented 3 months ago

@GrinZero sorry, I fixed it.

Brendan-csel commented 2 months ago

Wow - we used mobx with react for years and ...then moved to Solid so no longer need mobx.

I'd be curious how it is even possible to interop the two of them.

Also, I can't open your CodeSandbox either. It just sits on "Loading" in the browser tab.

westbrma commented 2 months ago

@Brendan-csel the codesandbox link works for me. Just curious but when you switched to Solid are you using createMutable for your state objects? This is how you use mobx with solid, see the enableExternalSource call: https://codesandbox.io/p/sandbox/mobx-external-source-0vf2l?file=%2Findex.js

Brendan-csel commented 2 months ago

We use createMutable in some places - mostly short-lived like to hold a record while it is being edited by a complex form. It is good for scenarios where there is a complex record of possibly unknown shape, many of the fields need to be individually reactive, AND you want to be able to update it identically to a plain object.

It's surprising how often you can get away with just plain signals too though. Sometimes we only need a couple of fields to be reactive in a record so we've got a utility function to add backing signals and swap out the field for a getter/setter - for all those cases when it feels too heavy to just createMutable the whole thing (like lists). But if you're bringing in the whole of mobx to work with Solid, you're probably not worried about "too heavy" :)

Brendan-csel commented 2 months ago

Yeah that original link is showing me a 404 in the console for /api/v1/sandboxes/mobx-external-source-forked-23rpd2. Maybe it's cached on your PC, or its a security thing.

Anyway, we found it pretty easy to refactor from mobx - although we we're still using the decorator syntax.

It also gave more opportunity to reconsider the API exposed from "state objects" - like this version of the Timer example that auto-disposes and only exposes a read-only secondsPassed and reset to the app code.

westbrma commented 2 months ago

@Brendan-csel ah ok I opened the link in incognito window and it would not load, I never used that codesandbox before, I had to change the view access to public. Its working now. The beauty of mobx for me is that you could pass any state object into makeAutoObserable() and it would be deeply reactive without using any special getter/setter functions. I don't care for the extra boilerplate but people argue its a "safer" design.

Brendan-csel commented 2 months ago

Disclaimer - I'm just hacking on this from reading the source.

I see there is a second parameter to enableExternalSource to provide an untrack function. Conveniently mobx provides one of those called untracked.

So adding that like below seems to solve the issue for me...

enableExternalSource(
  (fn, trigger) => {
    const reaction = new Reaction(`externalSource@${++id}`, trigger);
    return {
      track: (x) => {
        let next;
        reaction.track(() => (next = fn(x)));
        return next;
      },
      dispose: () => reaction.dispose(),
    };
  },
  (fn) => untracked(fn)
);

EDIT: I also just stumbled on this previous issue that might give you some other tips: https://github.com/solidjs/solid/issues/1850

EDIT2: also make sure you are using a recent version of Solid. The CodeSandbox package.json version was very old.

westbrma commented 2 months ago

@Brendan-csel good find! however as with #1850 using untracked breaks all my array updates inside

ryansolid commented 2 months ago

I'm going to close this as duplicate of #1850.. But yeah we will track array length in the next version(releasing in a few mins) so you should be good.