React-apollo: Default notifyOnNetworkStatusChange to true

[lorensr]

Regarding: http://dev.apollodata.com/react/api-queries.html#graphql-config-options-notifyOnNetworkStatusChange

I don't know whether it makes sense to change the default behavior of the core client:

http://dev.apollodata.com/core/apollo-client-api.html#ApolloClient.watchQuery

But I think it at least makes sense to change for React. Coming from React, I expect my component to be re-rendered when my props change. For example if I'm querying for Posts, I get data.Posts, and if I poll or fetchMore, the component gets re-rendered with a new data.Posts. However, it does not get re-rendered when data.loading or data.networkStatus changes, which leads to questions like #727.

Maybe it's better to meet dev expectations by re-rendering by default? And those concerned with performance can find a change notifyOnNetworkStatusChange to false.

[stubailo]

I think my main concern here is people having troubles with their components rerendering too often. It would be rough if people felt like adding Apollo makes their app inefficient, without looking into the options to turn it off.

[stubailo]

Of course, we _should_ rerender whenever loading changes, which is independent of network status. Also, we should make sure not to provide networkStatus at all if that option is not passed. Not sure if either of the above are problems.

[lorensr]

Ah, thanks. To be clear, you're saying that if options.notifyOnNetworkStatusChange is undefined or false, then data.loading should be undefined? I've observed options.notifyOnNetworkStatusChange: undefined -> data.loading: 1.

[stubailo]

No - data.loading should always be present, and always be either true or false.

But data.networkStatus should be undefined if options.notifyOnNetworkStatusChange is not set.

[lorensr]

Yes sorry meant networkStatus. Should I open separate issue for that and close this one?

[stubailo]

Yeah we can also rename this one 🙂

[lorensr]

Is that your pref in general? Curious for maintenance in the future—seems clearer/simpler to me to have a separate issue because otherwise you have to edit the first comment to explain the new issue, and people have to figure out which comments they have to pass over in order to get to the discussion about the current version of the issue.

[stubailo]

I guess I don't really care, but the opposite opinion says that this thread already contains the context about how we came to this conclusion.

[helfer]

notifyOnNetworkStatusChange is used to determine whether or not the component gets new props when the network status changes. Setting it to false doesn't currently mean networkStatus will be undefined.