[Experimental][WIP] Refactor api

[mweststrate]

Follow-up experiment based on discussion in the MobX 6 branch, removing a lot of api's to keep things OSS and undo some pollution preventing this to becoming a second mobx-react:

1. kill useAsObservableSource, and all complexity that is introduced with it. Instead, but the props (or whatever) directly into the observable and
   2. use useEffect to update the observable after receiving new props
   3. or, to optimize away the possible double render, directly assign the observable update for render. This works fine for observables that are only used locally, but will, of course, trigger warnings when sending the observable to children, so 1) is recommended
2. kill the second arg of useLocalStore (the source)
3. to avoid statically hard to detect bugs caused by any existing second argument to be interpreted as annotations, renamed it to useLocalObservable. But maybe we should keep it as is to simplify migration, I think that second arg is rarely used?
4. killed the forceUpdate customisation option. Features should be useful for basically everyone or nobody, not a single lib
5. killed useObserver to encourage people to use Observer / observer. This might be a bit too aggressive and probably better to export it as _useObserver or with a deprecation warning

I think this is such a big change, that we should release it as separate major after releasing mobx 6 compatibility, so that people don't have to migrate immediately?

[mweststrate]

... although a simpler migration would probably be to still expose useLocalStore with deprecation message, and useAsLocalSource that has a deprecation message + the old naive implementation, basically:

useAsLocalSource(p) {
    useMemo(() => {
       console.warn("bla bla deprecated")
     })
    const o = useLocalStore(() => p)
    Object.assign(o, p)
    return o
}

[urugator]

Imo if we keep useLocalStore, then people will keep using computeds, that depend on local vars and it will be the same discussion over and over ...
Moving local store outside the component is the way to prevent these issues. I suppose people are using useLocalStore(initStoreFn) anyway, so basically nothing really changes, it becomes const [observable] = useState(initStoreFn).
They will simply use exactly the same tools for defining observable stores as they would anywhere else, no need to introduce anything new.
Besides useLocalStore becomes a one liner, so if someone for unknown reason cannot live without it and feels confident enough about himself and his colleagues, he's free to introduce it to their codebase.

[coveralls]

Coverage decreased (-0.9%) to 94.536% when pulling ad6ba9d on refactor-api into 7f99f3b on master.

[mweststrate]

I think the hook has a few benefits over the useState hook: move people away from using setState and forcing to use an initializer function, and auto-binding.

[danielkcz]

I am still not convinced about removing useObserver. For fun, I tried searching that name over the GitHub, and 5000 results is not exactly a small number. And that's not considering private repos, etc... We might break a lot of codebases here without a clean way out of such breakage.

I even looked at some of my old codebases I no longer maintain and that hook is used fairly heavily too. It will be a real pain to refactor that.

Is it really worth just to "protect" people who don't know what they are doing?

[mweststrate]

Doh, intended to rewrap useObserver with a deprecation message in index, but forgot about that🤦‍♂️

[danielkcz]

Please consider again deprecating useObserver. Did you miss my comment above?

[mweststrate]

5K isn't really a big number, e.g. in comparison "observer mobx" gives 300K results. For old project it is no problem, as they use old versions of deps as well, and even with a new version it is just a single deprecated message that will only show up in DEV. I think if people use an unrecommended, soon to be undocumented api, it is no more than fair to point to them it is deprecated and how to fix it.

Note that moving to MobX 6 will already introduce much larger changes than replacing useObserver with <Observer>, like needing to add constructors everywhere.

Let's deprecate first, and we can always see if the fallout is too big to revert that message. But for now the balance between the support load / confusing it generates versus the unique benefits it yields don't justify its existence to me.

[danielkcz]

Let's deprecate first, and we can always see if the fallout is too big to revert that message. But for now the balance between the support load / confusing it generates versus the unique benefits it yields don't justify its existence to me.

Ok, sounds fair enough.

But let's definitely keep useLocalStore or even a different name (useLocalObservable?). Just in case the useObserver stays so it doesn't clutter with useObservable as it was in the past.

[mweststrate]

useLocalObservable sounds great!

…

On Fri, Aug 28, 2020 at 2:38 PM Daniel K. ***@***.***> wrote: Let's deprecate first, and we can always see if the fallout is too big to revert that message. But for now the balance between the support load / confusing it generates versus the unique benefits it yields don't justify its existence to me. Ok, sounds fair enough. But let's definitely keep useLocalStore or even a different name (useLocalObservable?). Just in case the useObserver stays so it doesn't clutter with useObservable as it was in the past. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#306 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4NBC3HCFSAVPQXWEE5T3SC6XNJANCNFSM4QMKTCKA> .

[danielkcz]

Hm, this is cool, but isn't it too confusing? I mean why to bother with mobx-react in here? If people want to use mobx-react-lite only they need to care about MobX version only, right? Some might get an impression from this table they also need to install mobx-react of a specific version 🤷‍♂️

[mweststrate]

yeah your right, this should probably be somewhere else :)

[danielkcz]

That [0] is hard to spot, people are more used to tuple syntax imo.

Suggested change

	`const state = useState(() => observable(initializer(), annotations, { autoBind: true }))[0]`
	`const [state] = useState(() => observable(initializer(), annotations, { autoBind: true }))`

[mweststrate]

agreed

[danielkcz]

I think we should deprecate this name and find something that doesn't look like a hook :) Something like enableStaticRendering or even enableSSR.

[danielkcz]

Personally, I wouldn't keep this whole section here at all. Or do you actually want to keep these around in V3 and remove it in a future V4?

[mweststrate]

Yeah wanted to keep them around in v3, to not make migration too hard, and remove them in v4 only. Also nice to have the docs somewhere available in case people are looking at old code. So I think it is probably not too much in the way here.

[danielkcz]

Maybe add a short explanation that this is done automatically now and they don't need to worry about it?

[mweststrate]

See next line :)

[kubk]

To be honest, I don't understand what is the point of useLocalStore. Using useState + factory function to create a store makes more sense because it makes store logic decoupled from React & UI. Example:

class Counter {
  value = 0;

  constructor() {
    makeAutoObservable(this);
  }

  increase = () => this.value++;
  decrease = () => this.value--;
}

const Component = observer(() => {
  const [counter] = useState(() => new Counter());
  ...
})

In this scenario, it is clear how to cover such store with unit tests. How to do it with useLocalStore? I also use Mobx strict mode and useLocalStore isn't compatible with this mode, because it doesn't convert methods to actions. It only converts methods to transactions.

[danielkcz]

I also use Mobx strict mode and useLocalStore isn't compatible with this mode, because it doesn't convert methods to actions. It only converts methods to transactions.

This will work correctly with MobX 6

If you follow through comments here, we are going to rename useLocalStore to useLocalObservable and it will be just tiny wrapper around observable call with some defaults. Not everyone likes overly verbose class stores.

[mweststrate]

Merging into mobx6 branch for a more holistic diff