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

(Philosophical question) Make dependency more explicit? #937

Open
fzyzcjy opened this issue Sep 30, 2023 · 13 comments
Open

(Philosophical question) Make dependency more explicit? #937

fzyzcjy opened this issue Sep 30, 2023 · 13 comments

Comments

@fzyzcjy
Copy link
Collaborator

fzyzcjy commented Sep 30, 2023

As we know, riverpod is quite popular these days. I have been using mobx for a long time and think it is quite good, but I also see some strengths of riverpod. Therefore, I create this thread as a brainstorm :)

One thing I think interesting is its explicit dependency shown in code. It seems like they use ref.watch(something), when mobx just read the something directly. (I am not an expert in riverpod, so please correct me if I am wrong.)

At first, I felt mobx's way is much neater and shorter, and it does give productivity. When the codebase has grown large, I sometimes also want to know what part will cause the downstream to react. In addition, IIRC once in a while it is possible to forget to put an Observer, and then everything does not react. For example, this issue can happen when doing refactor to extract some subtree of widgets into a new widget - and then forget to put an extra observer. Similar issues are, for example, when you have an Observer, and later refactor and add a LayoutBuilder/Builder/whatever - then everything inside the builder will not be reactive without an extra Observer.

So a very very naive change to mobx may be: Add ref as an argument to Observer(builder: (context, ref) => ...). For @observable, maybe enhance the codegen such that it gives some reader object that can be used like ref.watch(store.myfield). For @computed, maybe something similar (since again we use codegen).

There do exist drawbacks for the approach. For example, people using it for small projects may feel that there are extra code to be typed.

@MaoHolden
Copy link

MaoHolden commented Sep 30, 2023

This reminds me of Cerebral.js, they have a Single State with arbitrary depth and you specify which parts of the state you want to listen to:

import React from 'react'
import { state, sequences } from 'cerebral'
import { connect } from '@cerebral/react'

export default connect(
  {
    foo: state`foo`,
    onClick: sequences`onClick`
  },
  class MyComponent extends React.Component {
    render() {
      return <div onClick={() => this.props.onClick()}>{this.props.foo}</div>
    }
  }
)

Its very nice, very intuitive and specific IMO. But maybe make it optional? In small and medium projects just using the builder without worrying what store do you need is very nice.

I have a store of stores with something resembling a Single State so I just do:

        Observer(
          builder: (context) {
            if (state.wallet.withdraw.isFetchingWithdrawalInfo) {
              return const CustomProgressIndicator();
            }
            // if (state.otherStore.someObservable) ...
            
            return Text('${state.wallet.withdraw.userBalanceShib}');
          },
        )

I'm keeping it as simple as possible, so I would argue that if this change is introduced would be nice to make it optional. It adds too much code to my simple app. :)

@MaoHolden
Copy link

@fzyzcjy what about creating a linter rule to avoid using observables without Observer( inside a widget 🤔

@fzyzcjy
Copy link
Collaborator Author

fzyzcjy commented Oct 1, 2023

@MaoHolden

I would argue that if this change is introduced would be nice to make it optional. It adds too much code to my simple app. :)

Definitely optional - I am not going to introduce any breaking changes! And this is just a brainstorm - far from real implementation.

what about creating a linter rule to avoid using observables without Observer( inside a widget

That will be quite hard indeed :/ It is possible to call a normal function, which calls a normal function, etc, and finally deep nested it fetches an observable.

@MaoHolden
Copy link

@fzyzcjy could this be related to that?

always: If observables are read outside actions/reactions, throw an Exception never: Allow unrestricted reading of observables everywhere. This is the default.

https://pub.dev/documentation/mobx/latest/mobx/ReactiveReadPolicy.html

@MaoHolden
Copy link

Looks like the reactive context has policies for when you can read/write observables, and looks like its possible to tell mobx to throw an error if an observable was read outside a reactive context, like an action or a reaction.

https://mobx.netlify.app/api/context/

@MaoHolden
Copy link

@fzyzcjy just realized you are contributor lol, you already know all of these things. 🙈

@fzyzcjy
Copy link
Collaborator Author

fzyzcjy commented Oct 1, 2023

Yes, I know that. That one is great, but it is caught at runtime (instead of compile time), and there is no syntax hint to show explicit dependency (whether explicit dependency is a burden or a help is debatable!).

Again, this thread is a brainstorm, instead of any actual proposal to really implement something :) I just have something in my mind and thus output it here in case it is interesting for anyone.

@MaoHolden
Copy link

Got it, so Reactive Policies are enforced at runtime, that's not on the docs, good to know.

Then it definitely ads more safety, probably would also make mocking tests for individual widgets very easy since you know the store dependencies beforehand right?

@pavanpodila
Copy link
Member

We have to spend time on improving the documentation. I wrote the first version few years back and need help to revisit and make it more comprehensive.

@MaoHolden
Copy link

@pavanpodila I'm reading the codebase and trying to make sense of it, after I get an idea of how this works I'll try to do some PRs for the docs. Is there anywhere you discuss documentation stuff?

@pavanpodila
Copy link
Member

No location yet but we can start a separate thread. Look forward to your PRs

@subzero911
Copy link
Contributor

So a very very naive change to mobx may be: Add ref as an argument to Observer(builder: (context, ref) => ...). For https://github.com/observable, maybe enhance the codegen such that it gives some reader object that can be used like ref.watch(store.myfield). For @computed, maybe something similar (since again we use codegen).

@fzyzcjy
It's great. But Observers' advantage is to track several observables at once.
With ref it wouldn't be possible anymore.

@subzero911
Copy link
Contributor

subzero911 commented May 22, 2024

@fzyzcjy you can save the current behaviour, just rename current Observer into AutoObserver. But it would be a breaking change, you should add a migration note, that existing projects should now find-and-replace all Observers to AutoObservers in their codebases.
And also add Observer, Observer2, Observer3... Observer8, for explicit depencencies. It would be similar to https://pub.dev/documentation/provider/latest/provider/Selector-class.html

So people can choose, and those who want to let MobX track the observables by itself will use an AutoObserver, but ones who want to subscribe to observables explicitly will use a manual Observer.

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

No branches or pull requests

4 participants