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

Get rid of the suppressChangedStoreWarning flag leftovers #742

Merged
merged 2 commits into from
Aug 1, 2019
Merged

Get rid of the suppressChangedStoreWarning flag leftovers #742

merged 2 commits into from
Aug 1, 2019

Conversation

vkrol
Copy link
Contributor

@vkrol vkrol commented Aug 1, 2019

This flag was deleted in 6.0.0 but there are some leftovers. The quote from the 6.0.0 changelog:

The suppressChangedStoreWarning flag for Provider has been dropped.

and

Changing the set of stores in Provider is no longer supported and while throw a hard error (this was a warning before)

@danielkcz
Copy link
Contributor

danielkcz commented Aug 1, 2019

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.

@vkrol
Copy link
Contributor Author

vkrol commented Aug 1, 2019

I would like to wait for @mweststrate comment about this restriction. I'd like to suggest the following:

  1. We are merging this PR (I want to merge this as soon as possible because we have the wrong information about this flag in README.md).
  2. I'm creating an issue in which we're waiting for @mweststrate comment.

@vkrol
Copy link
Contributor Author

vkrol commented Aug 1, 2019

I want to merge it anyway, because in theory we'll have to wait a long time for comment.

README.md Outdated Show resolved Hide resolved
Co-Authored-By: Daniel K. <[email protected]>
@danielkcz danielkcz merged commit 8613c09 into mobxjs:master Aug 1, 2019
@vkrol vkrol deleted the get-rid-of-suppress-changed-store-warning-flag-leftovers branch August 1, 2019 20:26
@vkrol
Copy link
Contributor Author

vkrol commented Aug 2, 2019

I'm creating an issue in which we're waiting for @mweststrate comment.

#745

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

Successfully merging this pull request may close these issues.

2 participants