-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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 RelayRenderer isomorphic #589
Comments
Great work on // SERVER
Relay.injectNetworkLayer(new RelayServerNetworkLayer(...));
Relay.prepare(route, component, ({props, data, error /*, ...*/ }) => {
if (error) {
// stuff...
}
React.renderToString(
<YourPageTemplate
data={data}> // opaque data to be passed as JSON to the client
<RelayRenderer {...props}>
</YourPageTemplate>
);
});
// CLIENT
Relay.injectNetworkLayer(new RelayClientNetworkLayer(data)); // pass in the data from the server
ReactDOM.render(
<RelayRenderer route={...} component={...} />,
...
); It would be relatively simple to change Any interest in submitting a PR? :-) |
Thanks! Sure, I'll be glad to submit a PR. How about triggering synchronous rendering in I think, the first render on the client should also be synchronous, otherwise server side rendered DOM will not match DOM after the first render on the client (it will be just |
Good point, injecting the data from the server into the client's network layer means we can't synchronously render on the client. It might be reasonable to adjust the above such that // SERVER
// note: use the normal network layer
Relay.prepare(route, component, ({props, data, error}) => {/* see above */});
// CLIENT
// note: use the normal network layer
// `data` is intentionally opaque: it contains the serialized queries executed on the server
// and their corresponding response payloads (i.e. the inputs to `RelayStoreData.handleQueryPayload`).
Relay.injectPreparedResponse(data);
// Rendering will be synchronous so long as the client uses identical variables as the server
ReactDOM.render(
<RelayRenderer route={...} component={...} {...props} />
...
); |
In isomorphic-relay, on the client I use I like the symetry of
What about incorporating a similar implementation? |
The README contains info about how |
Yeah,
But the implementation can also be simplified. Below is a more complete API specification - we'd happily accept contributions along these lines! /**
* Part 1: Add `prepare` - this should intercept calls to `handleQueryPayload`,
* record the queries and payloads, and make them available as a `PreparedData`
* array in the callback. The main challenge here is determining a suitable injection
* point, an injected network layer would be the easiest place, but ideally `prepare`
* would not require one.
*/
Relay.prepare(
route: Relay.Route,
component: Component,
preparedStateChange: PrepareStateChange
): void;
/**
* Part 2: Add `injectPreparedData` which accepts the queries and payloads from the
* server and writes them into the store. (use `RelayStoreData#handleQueryPayload`).
*/
Relay.injectPreparedData(data: PreparedData): void
/**
* Part 3: Make `RelayRenderer` render synchronously if passed the `props` from the
* `prepare` callback.
*/
<RelayRenderer {...prepareState.props} />
// Types for the above:
type PreparedData = Array<{
query: ConcreteQuery;
result: ?Object;
}>;
type PrepareState = {
props: Object;
data: PreparedData;
error: ?Error;
};
type PrepareStateChange = (prepareState: PrepareState) => void; |
cc @yungsters |
Sorry for delays. I wish I were in the same time-zone as you.
Do you just mean that Component-route pair can resolve to multiple
Yeah, isomorphic-relay intercepts calls to But probably a better place to intercept queries is right in the |
Seems like that to recreate a type PreparedData = Array<{
query: ConcreteQuery;
routeName: string;
variables: Variables;
result: ?Object;
}>; Then the implementation of const storeData = RelayStoreData.getDefaultInstance();
function injectPreparedData(data) {
data.forEach(([{query, routeName, variables, result}]) => {
const rootQuery = RelayQuery.Root.create(
query,
RelayMetaRoute.get(routeName),
variables
);
storeData.handleQueryPayload(rootQuery, result);
});
} It mostly works, but there is a problem: when I tried to |
More than that: plural root fields (
This seems reasonable for an initial implementation, and we can discuss details more on the PR.
Yeah it seems that way, but it actually isn't necessary. |
Now I see. I have found that deferred queries are split here: https://github.com/facebook/relay/blob/v0.5.0/src/legacy/store/GraphQLQueryRunner.js#L162-L170 And as far as I understand, we have to match intercepted split queries with the original ones returned by P.S. |
There's no need to match up the queries and results from the server with those on the client. So long as the
That's a good question - this should be configurable by the product as it can have a significant impact on initial load time. In the case that the server does not prepare deferred data, the client will be able to render without it (it doesn't wait for deferred data) and then send requests for the additional deferred data. |
Sorry, I was not clear. I meant matching on the server. It will serve multiple requests in parallel ( |
The server just has to record the (query, response) pairs written to |
But how will we know to which HTTP-request this intercepted (query, response) pair belong? |
Probably, intercepting in |
This isn't necessary. If the server records all pairs of (query, response) that are written to the store and injects this data on the client, then the client won't even need to make network requests. Intercepting |
And this is how isomorphic-relay already works. But I was talking about server side only. Consider a hypothetical implementation of const queryPayloadSubscribers = new Set();
const store = RelayStoreData.getDefaultInstance();
store.injectQueryPayloadInterceptor((query, payload) => {
queryPayloadSubscribers.forEach(subscriber => {
subscriber(query, payload);
});
});
function prepare(route, component, preparedStateChange) {
const querySet = getRelayQueries(component, route);
const data = [];
queryPayloadSubscribers.add(queryPayloadSubscriber);
RelayStore.forceFetch(querySet, ({aborted, done, error, stale}) => {
if (aborted) {
error = new Error('Aborted');
}
if (done && !stale || error) {
queryPayloadSubscribers.delete(queryPayloadSubscriber);
preparedStateChange({props: {...}, data, error});
}
});
function queryPayloadSubscriber(query, payload) {
if (isQueryBelongToThisRequest(query)) {
data.push({query: toGraphQL.Query(query), result: payload});
}
}
} Note that multiple requests might be processed concurrently because In isomorphic-relay I just check if the |
Ohhh - you're assuming one global Relay context shared amongst multiple HTTP requests, and trying to distinguish which query/payload goes with which. I'm assuming that we can have a unique Relay context per HTTP request - see #558 which is moving along rapidly. |
Local Relay context would be a blessing. I have been able to use some enhancements from #558 in isomorphic-relay already, but with some hacks: https://github.com/denvned/isomorphic-relay/blob/v0.3.0/src/prepareData.js Probably, we can consider that as a prototype of And https://github.com/denvned/isomorphic-relay/blob/v0.3.0/src/injectPreparedData.js as a prototype of |
Working on the PR now. I hope to make it ready this week. |
I thought a lot about this recently, and came to the conclusion that passing special properties to
Instead, we could simply make The question is how we will implement it.
Also, in the server-side rendering mode, But, it is not too hard to modify I've already experimented with it by making // `RelayStoreData#createQueryRunner(querySet)` just returns
// `new GraphQLQueryRunner(this, querySet)`.
// The constructor of `GraphQLQueryRunner` prepare queries (i.e.
// diffs and splits them), and calculate the initial ready state based
// on availability of the data in the store cache.
const queryRunner = storeData.createQueryRunner(querySet);
// Synchronously get the initial ready state to check the cache:
const readyState = queryRunner.getReadyState();
// Perform initial render
...
// If we still need to send the queries to the GraphQL server
// asynchronously:
queryRunner.run(callback); Is it OK if I will work on the PR along these lines? |
This is conceptually simple, but RelayRenderer checks the cache asynchronously (after mount) explicitly because that operation can be expensive and block rendering.
There are existing functions in Relay for synchronously checking if the results of a query are cached - there's no need to refactor the QueryRunner API for this.
|
That is not quite true. Currently, (I am aware, that there are also asynchronous calls to
Yes, but there is a problem. If we check cache aside from
Fortunately, changes to Also, if any changes to the API of |
Here is my work on isomorphic It uses #625 as a base. @josephsavona as you suggested I added to So, it is enough to set the <RelayRenderer
Component={Component}
queryConfig={queryConfig}
prepared={true}
/> But you should examine the source to see the details. It looks pretty solid for me. Is that good for a PR? TODO: tests for the new mode. |
Hi @josephsavona, I am currently working on a project that will need this feature; and wondering if the above mentioned PR is a feasible solution? or what needs to be done to make progress on this feature? and thanks @denvned for your work 👍 |
@josephsavona , @yungsters Looks like it is not possible anymore because of a26c8b4. Is it possible to revert that commit back? It will be hard to implement |
@denvned Sorry about that. I have a revision awaiting internal review that will bring this back. |
@yungsters please add a note about |
@yungsters Thanks for bringing |
Summary:...to make sure `RelayRenderer` does not run queries during synchronous server-side rendering. In the `RelayRenderer` tests, I had to replace `ShallowRenderer.render` with the traditional `ReactDOM.render`, because `ShallowRenderer.render` does not invoke some component lifecycle methods including `componentDidMount` that now run queries. But I did not change the logic of the tests, as **visible behaviour of `RelayRenderer` was fully retained.** Added a test to check that `RelayRender` does not run queries on the server. This PR is a forerunner of the PR that will make `RelayRenderer` isomorphic (see #589). That next PR will be pretty small and easily comprehensible, because this PR already did much of the necessary dirty work. Closes #625 Reviewed By: josephsavona Differential Revision: D2727748 fb-gh-sync-id: 202cc772d15661532afd5eee6ae647bb03af9dcc shipit-source-id: 202cc772d15661532afd5eee6ae647bb03af9dcc
This has been implemented as |
@josephsavona In isomorphic-relay I have managed to implement But some other parts of isomorphic-relay still have to use few Relay internals, namely |
@denvned thanks for confirming. We should add public methods for serializing/deserializing queries (the to/fromGraph stuff). |
I know, but is |
I could make a PR based on this: https://github.com/denvned/isomorphic-relay/blob/v0.2.1/src/IsomorphicRenderer.js
Currently,
_runQueries
is an async operation, so it should not be called during server side rendering. That's why I moved it tocomponentDidMount
, which is not called on the server.isDataReady
checks if the data has been already fetched (current implementation is not so important, and might be replaced). If the data is available, initialize with a preloaded state, otherwise with an empty state.What do you think?
The text was updated successfully, but these errors were encountered: