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

Make RelayStore Contextual #603

Closed
wants to merge 1 commit into from
Closed

Make RelayStore Contextual #603

wants to merge 1 commit into from

Conversation

devknoll
Copy link
Contributor

This is a cleaner shot at #570.

  • RelayGarbageCollection hasn't been updated yet (needs clarification)
  • RelayRenderer takes RelayContext as a prop (instead of as context from RelayRootContainer).
  • Not using Joe's newest GraphQLStoreQueryResolver changes.
  • Adds Relay.Store deprecation calls.
  • Fixes RelayStore mock.

This ignores the clean up of interdependencies in RelayStoreData, since the discussion on that is ongoing.


this.props.relayContext.injectBatchingStrategy(
ReactDOM.unstable_batchedUpdates
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ReactDOM.unstable_batchedUpdates is not suitable for server-side rendering. And generally, I believe RelayRenderer should not be responsible for defining the batching strategy, and should not override it either. It may cause problems such as: denvned/isomorphic-relay-router#5 (comment). Still, ReactDOM.unstable_batchedUpdates might be the default injected by the constructor of RelayContext.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or even better, do not inject ReactDOM.unstable_batchedUpdates at all. The user can inject that himself if he needs it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be good for Relay to have a sensible default here... Maybe we could introduce a BatchingStrategyLayer that by default injects ReactDOM.unstable_batchedUpdates on the client side, etc. Then you can override this if you'd like.

(In this particular case, the change was made to keep the same behavior as master.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the following can solve the problem temporarily, and be compatible with master:

RelayStore.injectBatchingStrategy(
  ReactDOM.unstable_batchedUpdates
);

But, since you are deprecating RelayStore the following might be better:

const defaultContext = RelayContext.getDefaultInstance();
defaultContext.injectBatchingStrategy(
  ReactDOM.unstable_batchedUpdates
);
RelayRenderer.defaultProps = {
  context: defaultContext,
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the later approach have a problem that only the default instance will have the batching strategy injected. Injecting ReactDOM.unstable_batchedUpdates to every new RelayContext by default is also bad, because in couples Relay to ReactDOM too much.

I think, that could be solved by subclassing RelayContext with ReactDOMRelayContext, so only the later will inject ReactDOM.unstable_batchedUpdates. And make ReactDOMRelayContext the default for the context property of RelayRenderer:

RelayRenderer.defaultProps = {
  context: ReactDOMRelayContext.getDefaultInstance(),
}

And make RelayStore an alias of ReactDOMRelayContext.getDefaultInstance() for backward compatibility.

cc @josephsavona

@denvned
Copy link
Contributor

denvned commented Nov 16, 2015

RelayRenderer should abort the pending request and run new queries when the Relay context is replaced.

RelayContainer should also react to the change of the context.

@devknoll
Copy link
Contributor Author

Good catch 👍 Thanks for the feedback!

};

RelayRenderer.defaultProps = {
relayContext: RelayStore,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you are deprecating RelayStore, may be RelayContext.getDefaultInstance() is better?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

relayContext is a required prop, there's no need for a default

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default was added here because tons of tests are shallow rendering a RelayRenderer. Will update those when I get a chance 👍

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

Successfully merging this pull request may close these issues.

4 participants