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

README Enhancement: Initial store state is by default an empty object #5

Closed
nhardy opened this issue Mar 25, 2016 · 9 comments
Closed

Comments

@nhardy
Copy link
Contributor

nhardy commented Mar 25, 2016

Something that isn't immediately obvious from the README is that the proxy Store() by default, will initialise the state as an empty object. This causes issues with react-redux if you are accessing nested properties in the state.

Trawling through the code, however, https://github.com/tshaddix/react-chrome-redux/blob/b29d0e9b352bea1ac999415a73f0521eb63327f3/src/store/Store.js#L9 shows that there is an option to initialise a default state before this proxy Store receives state information from the wrapped Store. This is practical for small projects, and should be added to the README.

For larger, more complex initial states though, it may be more practical to wait until the proxy Store receives its initial state. See example below:

const unsubscribe = store.subscribe(() => {
   unsubscribe();
   render(
    <Provider store={store}>
      <App/>
    </Provider>
    , document.getElementById('app'));
});

This example ensures that the App is only rendered upon receiving the initial state from the wrapped store. I think this would also be worth adding to the README.

@tshaddix
Copy link
Owner

@nhardy Great idea. This pattern allows for the default state logic to be declared in a single place. I think it's a valid thing to include in the README section regarding initialization. Heck, the entirety of the API should be included in the README :)

@tshaddix
Copy link
Owner

Now included in Advanced Usage docs. Thanks!

@sb8244
Copy link

sb8244 commented Dec 1, 2016

Just an FYI. It appears that there is a race between initializing the store and having the first state sent to you. If you subscribe after the port connected, you won't receive the callback. store.state can be checked for existing keys at the time of subscription to know if your state is initialized.

@tshaddix
Copy link
Owner

tshaddix commented Dec 1, 2016

@sb8244 Thanks for reporting this! Can you expand a bit more on the problem/solution? Having trouble picturing it for some reason. Maybe because it's Thursday.

@vhmth
Copy link
Collaborator

vhmth commented Dec 1, 2016

@tshaddix I believe he's saying:

  1. Create Store instance, which runs chrome.runtime.connect
  2. Wait a while
  3. The port from step 1 connects and is sent the state.
  4. Call subscribe on store.

For posterity:

https://github.com/tshaddix/react-chrome-redux/blob/master/src/store/Store.js

So, as @sb8244 had mentioned, the solution here would be to check Object.keys(this.state) > 0 at time of the call to subscription and then send the listener the state:

subscribe(listener) {
  this.listeners.push(listener);

  if (Object.keys(this.state) > 0) {
    listener(this.state);
  }

  return () => {
    this.listeners = this.listeners.filter(l => l !== listener);
  };
}

@tshaddix
Copy link
Owner

tshaddix commented Dec 2, 2016

@vhmth Thanks for laying it out!

So this store is supposed to act exactly like a Redux store. Does redux send state on subscriptions?

I don't see anything here: https://github.com/reactjs/redux/blob/master/src/createStore.js#L101

@vhmth
Copy link
Collaborator

vhmth commented Dec 2, 2016

Seems like they don't handle it on redux, but connected container components get the state from the store immediately after subscribe.

https://github.com/reactjs/react-redux/blob/master/src/components/connect.js#L197

I'm not sure where we want to place the separation of concerns, but my gut would be to leave it as is and have this lib behave more like redux as you had suggested. That way, we can let react-redux handle the subscribe/initial store fetch.

@sb8244
Copy link

sb8244 commented Dec 2, 2016

I don't think that react-chrome-redux needs to handle this. I think that it is good to know about as a developer, but not something I would expect this to solve.

The async nature of this redux store implementation inherently means things might need to be done differently. There seems to be a reasonable choice over keeping interface with a regular store, or customizing. I'm totally cool with library maintainers choosing interface over custom exceptions.

FWIW:

    const unsubscribe = store.subscribe(() => {
      renderComponent(unsubscribe);
    });

    if (Object.keys(store.state) > 0) {
      renderComponent(unsubscribe);
    }

Also, this is a great library. Thanks for the work y'all do on it.

@vhmth
Copy link
Collaborator

vhmth commented Dec 2, 2016

Cool cool thanks for the clarification @sb8244. And for sure! This lib has absolutely saved our small little startup, so really I'm thankful to @tshaddix for laying down the groundwork. :-)

adamsanderson added a commit to adamsanderson/brook that referenced this issue Dec 30, 2019
As @sb8244 mentioned the example in the documentation does not account for the case where the store has already initialized before the subscribing.

Instead of fiddling around with subscribing and unsubscribing, we now take the declarative approach and watch the store, if it's empty we render an empty screen.

Related:
tshaddix/webext-redux#5

*: Hopefully.  The issue was most commonly visible when upgrading Firefox which isn't something that's easy to test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants