Skip to content
This repository has been archived by the owner on Dec 31, 2020. It is now read-only.

Idea: get rid of the restriction on changing set of stores in Provider #745

Closed
vkrol opened this issue Aug 2, 2019 · 15 comments
Closed

Comments

@vkrol
Copy link
Contributor

vkrol commented Aug 2, 2019

@mweststrate We had a discussion about the restriction on changing set of stores in Provider with @FredyC. What do you think about removing this restriction?

The quote from the change log for mobx-react@6.0.0:

  • Changing the set of stores in Provider is no longer supported and while throw a hard error (this was a warning before), as the model of Provider / inject has always been to inject final values into the tree. (That is, fixed references, injected objects themselves can be stateful without problem). If you want to dynamically swap what is provided into the tree, use React.createContext instead of Provider / inject. The suppressChangedStoreWarningflag forProvider` has been dropped.

a64c2d3#diff-4ac32a78649ca5bdd8e0ba38b7006a1eR11

I am thinking if this is still valid actually. Isn't it from legacy context too? I think it's because the legacy context could not propagate to pure components. The Context does not have that problem. I don't see a reason why the value could not change. I mean in MobX world it doesn't usually need to, but people are crazy sometimes :)

#739 (comment)

Looking at those links it's clear it's related to the legacy context and as such it's misleading.
I am really convinced we shouldn't prevent this if someone really wants that. There is no actual harm in that. It's not about recreating stores, just the context object itself and the only downside is it will re-render a whole tree from that Provider.

#742 (comment)

@urugator
Copy link
Contributor

urugator commented Aug 3, 2019

Afaik there shouldn't be any problems with propagation as the error suggests, I think the whole impl can be as simple as:

export function Provider({ children, ...newStores }) {
  const stores = React.useContext(MobXProviderContext);   
  return <MobXProviderContext.Provider value={{ ...stores, ...newStores }}>{children}</MobXProviderContext.Provider>
}

However, it will invoke all Injectors (or anything with useContext(MobXProviderContext)) any time the Provider renders ... which usually doesn't happen very often?

@danielkcz
Copy link
Contributor

danielkcz commented Aug 3, 2019

Another benefit of having such a simple implementation. People who peek into a source code will see there is no longer anything special about Provider and perhaps even migrate over to Context directly (which is the ultimate goal). Although inject is still pretty convoluted even though most logic is HOC related.

@danielkcz
Copy link
Contributor

danielkcz commented Aug 3, 2019

@urugator I wonder if we apply React.memo to Provider, isn't effectively the same as a current restriction? It won't throw an error, but the user can clearly see Memo in devtools and it's more predictable than some baked in condition.

As long as the user is passing props with the same reference there is no need to re-render Provider. If they pass something different, it will naturally allow it.

@urugator
Copy link
Contributor

urugator commented Aug 3, 2019

I wonder if we apply React.memo to Provider, isn't effectively the same as a current restriction

No, because Provider depends on children which are always different (unless they're memoized by parent, which is very unlikely). In other words Provider never recieves the same props.
You would have to perform the shallow check directly on the newStores object. Not sure if it's worth it. It may have both positive/negative impact on perf depending on how Provider/inject is being used.

@mweststrate
Copy link
Member

Wasn't really following the original convo, so might have missed it. But I am wondering: Is this there an actual use case where this is in the way? Otherwise, from personal experience, I'd keep it:

It is way way easier to start with a really narrow scope / use case, and extend it if needed. Compared to: supporting all possible use cases, and than noticing that you can't do certain things (for example, optimizations) because some patterns are already out there used in the wild :).

So I'd personally, if Provider is too limited to you, I would recommend just go for useContext right away. But I would keep the semantic meaning of Provider simple: it is designed for dependency injection, not for state management. State would better be managed in MobX observables :).

Hence my question: is there a specific use-case problem to be solved here. Or is it more about removing the limitation for the sake of removing the limitation? In the last case I'd recommend not to do it.

@danielkcz
Copy link
Contributor

danielkcz commented Aug 5, 2019

We bumped into this when @vkrol was doing a polish of unit tests and then started rewriting Provider to hooks on my suggestion and it looked suspicious there. We started digging and discovered it was there for legacy context which had a problem with it. Even the readme was mentioning it incorrectly.

Obviously, there is no problem with keeping it. It's just an idea to simplify code base so people who will peek under the hood will see there is nothing special about it.

@mweststrate
Copy link
Member

mweststrate commented Aug 5, 2019 via email

@urugator
Copy link
Contributor

urugator commented Aug 5, 2019

A little bit optimized impl (ref value created only once, grabStores replaced by destructuring):

export function Provider(props) {
    const parentValue = React.useContext(MobXProviderContext)
    const valueRef = React.useRef();
    if (!valueRef.current) {
      const { children, ...rest } = props;
      valueRef.current = {
        ...parentValue,
        ...rest,
      }  
    }         
    
    const value = valueRef.current;
    
    if (process.env.NODE_ENV !== "production") {
        const { children, ...rest } = props;
        const newValue = { ...value, ...rest } // spread in previous state for the context based stores
        if (!shallowEqual(value, newValue)) {
            throw new Error(
                "MobX Provider: The set of provided stores has changed. Please avoid changing stores as the change might not propagate to all children"
            )
        }
    }

    return (
        <MobXProviderContext.Provider value={value}>{props.children}</MobXProviderContext.Provider>
    )
}

EDIT (don't want to reopen, so just for completeness):

to simplify it to improve readability

export function Provider({ children, ...propsValue }) {
    const contextValue = React.useContext(MobXProviderContext)
    const [value] = React.useState(() => ({
      ...contextValue,
      ...propsValue,
    }));
             
        
    if (process.env.NODE_ENV !== "production") {        
        const newValue = { ...value, ...propsValue } // spread in previous value for the context based stores
        if (!shallowEqual(value, newValue)) {
            throw new Error(
                "MobX Provider: The set of provided stores has changed. Please avoid changing stores as the change might not propagate to all children"
            )
        }
    }

    return (
        <MobXProviderContext.Provider value={value}>{children}</MobXProviderContext.Provider>
    )
}

@danielkcz
Copy link
Contributor

danielkcz commented Aug 6, 2019

@urugator Do we really need to optimize Provider? :) I mean in most cases when it sits on top of the tree it won't even be re-rendered. And for other cases, it also doesn't happen that often unless someone is doing something terribly wrong.

We had it like that and I asked @vkrol to simplify it to improve readability because the benefit seems really unimportant. Am I missing something here?

I suppose let's close this, it was mostly just a wild idea.

@vkrol
Copy link
Contributor Author

vkrol commented Aug 6, 2019

It is way way easier to start with a really narrow scope / use case, and extend it if needed. Compared to: supporting all possible use cases, and than noticing that you can't do certain things (for example, optimizations) because some patterns are already out there used in the wild :).

So I'd personally, if Provider is too limited to you, I would recommend just go for useContext right away. But I would keep the semantic meaning of Provider simple: it is designed for dependency injection, not for state management. State would better be managed in MobX observables :).

Hence my question: is there a specific use-case problem to be solved here. Or is it more about removing the limitation for the sake of removing the limitation? In the last case I'd recommend not to do it.

@mweststrate I believe that if we want to keep the restriction as it is, we need to explain the motivation as clearly as possible in the documentation, because now it is not clear and confusing.

@danielkcz
Copy link
Contributor

@vkrol I mostly agree with you, but I would dare to say that most people won't ever encounter that exception unless they are doing something terribly wrong. That error message is kinda self-explanatory imo. Also, there is already some explanation in docs (see Notes below the example).

@urugator Noticed your edit. I was suggesting useState too, but @vkrol was against that... #739 (comment)

@vkrol
Copy link
Contributor Author

vkrol commented Aug 10, 2019

That error message is kinda self-explanatory imo.

No. The message is:

Please avoid changing stores as the change might not propagate to all children

But we don't know a single case where the change might not propagate.

Also, there is already some explanation in docs (see Notes below the example).

This note does not explain anything.

Conclusions:

  1. The error message is confusing and probably not even true.
  2. There is no explanation in the documentation and source code why this limitation exists.
    So I'm sure we need to either remove the restriction or describe in the documentation and error message the true reason for the restriction.

@danielkcz
Copy link
Contributor

danielkcz commented Aug 10, 2019

We should figure this one out first...

@mweststrate

I'm not sure the context changes would be picked up correctly by inject in all possible setups (e.g. named stores vs. injection function), or that those are covered by tests. So impicitly supporting this might actually introduce some bugs / kill some optimizations. (Definitely not sure about the last)

However, is it worth it? Consider that people will over time migrate away from inject, so investing some heavy dev time into it might be a waste.

or describe in the documentation and error message the true reason for the restriction.

So it's probably this one and to simply explain it's for a legacy reasons :) And in case someone actually runs into that scenario (which I doubt), we can start digging.

@vkrol
Copy link
Contributor Author

vkrol commented Aug 11, 2019

However, is it worth it? Consider that people will over time migrate away from inject, so investing some heavy dev time into it might be a waste.

I agree.

So it's probably this one and to simply explain it's for a legacy reasons :) And in case someone actually runs into that scenario (which I doubt), we can start digging.

OK, I will do the PR.

danielkcz pushed a commit that referenced this issue Aug 12, 2019
…f stores (#760)

* Clarify the reason why there is the restriction on changing the set of stores

Fixes #745

* Update README.md

Co-Authored-By: Daniel K. <FredyC@users.noreply.github.com>
@lock
Copy link

lock bot commented Jan 14, 2020

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs or questions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants