Make RelayRenderer isomorphic

[denvned]

The first commit of this PR does refactoring that simplifies state handling, but does not change behaviour. And the second, very simple one, actually makes RelayRenderer isomorphic.

Now it is possible to pass an initialReadyState to RelayRenderer, allowing server-side rendering and initial rendering in the browser to use pre-primed data.

See #589

[wincent]

I'd probably just use an if/else here rather than a many-line ternary.

[denvned]

But isn't it good practice to follow functional programming style when possible? Besides there is even fancier ternary in _runQueries. 😄

[josephsavona]

+1 for if/else

[denvned]

Not a problem, replaced with if/else. But it looks bit bulkier and harder to read, IMHO.

[everdimension]

No need for the else when returning conditionally.

[josephsavona]

@denvned Can you provide an example of how this would be used to implement server rendering? Would the new initialReadyState prop be used on the server, client, or both? It isn't obvious how this prop would be sufficient to render on the client given that data was fetched on the server. Data would have to be provided to the store from the server regardless.

As an alternative to this approach, how about creating an "isomorphic" RelayEnvironment implementation to be used as below. You could compose environment instances and override the implementation of key methods - forceFetch, primeCache - to skip work when data was already fetched:

// server
var serverEnvironment = new ServerRelayEnvironment(new RelayEnvironment());
React.renderToString(<RelayRender environment={serverEnvironment} ... />
var data = serverEnvironment.getDataForClient(); // <-- return to client

// client
dataFromServer; // from last step of server
var clientEnvironment = new ClientRelayEnvironment(
  new RelayEnvironment(), 
  dataFromServer
);
React.render(<RelayRenderer environment={clientEnvironment} ... />

[denvned]

@josephsavona

Can you provide an example of how this would be used to implement server rendering?

Sure, it mostly follows your recommendations in #589 (comment), and #589 (comment):

// server
const environment = new RelayEnvironment();
environment.prepareData(Container, queryConfig).then(
  ({props, data}) => {
    const reactMarkup = ReactDOMServer.renderToString(<RelayRenderer {...props} />);
    renderHtmlPage(reactMarkup, JSON.stringify(data));
  }
);

// client
const environment = new RelayEnvironment();
const data = JSON.parse(getData());
environment.injectData(Container, queryConfig, data).then(
  (props) => {
    ReactDOM.render(<RelayRenderer {...props} />, root);
  }
);

In both cases props are {Container, environment, initialReadyState, queryConfig}.

Would the new initialReadyState prop be used on the server, client, or both?

We need it on both, because the data should be ready on the client during the initial render, otherwise React wouldn't reuse server-side rendered markup.

override the implementation of key methods - forceFetch, primeCache - to skip work when data was already fetched

Actually, I have already tried that approach but unsuccessfully: #625 (comment). Also I think passing initialReadyState is more clean solution.

[josephsavona]

I see, thanks for clarifying. You're referring to your comment:

...React throws an error because onReadyStateChange calls setState, and React does not allow calling setState before mount.

This makes sense - RelayRenderer currently assumes rendering will be asynchronous, but if we're rendering on the client with data from the server it needs to be synchronous.

My concern here is that the shape and value of initialReadyState is an implementation detail that users shouldn't have to know about. However, exposing it as a prop makes it part of the public API and requires us to continue supporting it which we'd prefer to avoid.

I'd like to see a proposal that uses some form of composition in order to avoid the need to change the RelayRenderer API. For example, the API I proposed above could be achieved by creating an <IsomorphicRelayRenderer> that uses RelayRenderer internally except for the first render on the client (add whatever prop you need to indicate that the first render should resolve synchronously from pre-populated data). In general, we'd like to move towards providing lower level primitives (#559) and encourage composition over a monolithic core.

To summarize, rather than proceed with this PR I propose that we:

• Implement an IsomorphicRelayRenderer outside of react-relay so that users can do server rendering today
• Provide lower-level primitives to make isomorphic-relay easier to implement via all public methods.

@denvned what do you think?

[yungsters]

I have also been working on a lower-level component that RelayRenderer would compose. I came to the same conclusion as @denvned that feeding in a readyState object was sufficient. Where my line of thinking differed was that I was thinking of building a lower level RelayReadyStateRenderer that always synchronously rendered the supplied readyState.

@denvned Do you want to take a stab at this? Or want me to go ahead and build it (which is what I was planning on doing today)?

[denvned]

My concern here is that the shape and value of initialReadyState is an implementation detail that users shouldn't have to know about. However, exposing it as a prop makes it part of the public API and requires us to continue supporting it which we'd prefer to avoid.

I thought that it could be an undocumented, internals only prop that is passed exclusively as a part of opaque props returned by prepareData and injectData (as in my example above).

creating an that uses RelayRenderer internally except for the first render on the client

Nice idea! I think I even see all the details of a possible implementation (probably the trickiest part is to not allow RelayRenderer to destroy DOM tree on the second render on the client). But we need to make a public API in Relay that will allow to get props (containing fragment pointers) by a given Container and queryConfig that we then would pass to Container during the first render, or alternatively we probably can use RelayReadyStateRenderer proposed above by @yungsters to solve this.

Do you want to take a stab at this? Or want me to go ahead and build it (which is what I was planning on doing today)?

@yungsters, I can not wait to see it, but would not be able to build it myself today. So please do it! 👍

To summarize, rather than proceed with this PR

@josephsavona, I think the first commit of this PR is still useful, it cleans up RelayRenderer a lot without changing API or behaviour. Maybe you can cherry-pick it?

[josephsavona]

I thought that it could be an undocumented, internals only prop

I understand this line of thinking, and it's really tempting. Unfortunately if someone can pass it as a prop and depend on its behavior, they will, and this limits our flexibility. I really like @yungsters idea of a lower-level RelayReadyStateRenderer that you can use if you know what you're doing, and keeping a higher-level component that most people will use.

the trickiest part is to not allow RelayRenderer to destroy DOM tree on the second render on the client

I would imagine that this new component would render the root component directly at first, until the point that the props have changed and data needs to be fetched from the server. At that point you'd render a RelayRenderer and let it do the data-fetching (or possibly implement that yourself with other low-level APIs).

...Maybe you can cherry-pick it?

Agree that the first commit is worth discussing on its own; no rush, maybe split that off into a separate PR as time permits?

[denvned]

Agree that the first commit is worth discussing on its own; no rush, maybe split that off into a separate PR as time permits?

@josephsavona I've made a separate PR, #942.

[yungsters]

@denvned Does 8074aa0 suit your needs?

[denvned]

@yungsters Looks like it should. Thanks!