denvned / isomorphic-relay

Adds server side rendering support to React Relay
BSD 2-Clause "Simplified" License
242 stars 53 forks source link

injectPreparedData on the server side? #50

Closed jakepusateri closed 8 years ago

jakepusateri commented 8 years ago

Can prepareData accept on optional environment so that injectPreparedData can be called on the server side if some data is known to already be available, such as from an upstream service?

If not provided, environment can still default to new Relay.Environment().

If this seems like a reasonable approach, I can make a PR.

jwb commented 8 years ago

Definitely want to use such a capability. Hope a pull request would be accepted.

marcus-wu commented 8 years ago

Seems reasonable to me.

denvned commented 8 years ago

Can prepareData accept on optional environment

I didn't allow that because prepareData injects its own custom network layer, and such reconfiguration of an user's Relay.Environment would be dishonorable.

But the good news is that you don't need that functionality for your use case anyway:

const environment = new Relay.Environment();

...

// You don't need to pass the environment to prepareData:
const { data } = await IsomorphicRelay.prepareData(rootContainerProps, networkLayer);

// Instead just inject the prepared data to your environment:
IsomorphicRelay.injectPreparedData(environment, data);
jakepusateri commented 8 years ago

That approach does not work for me. The point of injecting the data on the server side is so that the network request would not have to be made. The approach I am using now is:

1) Create a GraphQL query that, if sent, would return the data I have. The data received is always the same shape, so this can be static.

const data = {
  firstName: 'Sam',
  lastName: 'Student',
  id: '123'
};
const query = Relay.QL`
query StudentRoute($id_0: ID!) {
  student(id: $id_0) {
    firstName
    lastName
    id
  }
}
`;

2) Convert that GraphQL query into a Relay query

const relayQuery = Relay.createQuery(query, { id_0: data.id });

3) Seed relay environment with this query, then render.

const environment = new Relay.Environment();
const storeData = environment.getStoreData();
storeData.handleQueryPayload(relayQuery, data);
const response = ({ query: toGraphQL.Query(relayQuery), response: data });
const networkLayer = new Relay.DefaultNetworkLayer('http://localhost:5000/');
const rootContainerProps = {
    Container: StudentPage,
    queryConfig: new StudentRoute({
        id: data.id
    }),
    environment,
    forceFetch: false
};
IsomorphicRelay.prepareData(rootContainerProps, networkLayer).then(({ data: relayData, props }) => {
    const reactOutput = ReactDOMServer.renderToString(
        <IsomorphicRelay.Renderer {...props} />
    );
    res.render(path.resolve(__dirname, 'views', 'index.ejs'), {
        preloadedData: relayData.concat(response),
        reactOutput
    });
}).catch(next);

This approach is currently working, and not sending any network requests if the components rendered do not need any additional data. #51 PRs the changes to this library needed.

denvned commented 8 years ago

The point of injecting the data on the server side is so that the network request would not have to be made.

Now I see what you are after. Still, allowing to pass an environment to prepareData would cause troubles. Probably a better approach here is to pass an optional array of preexisting query–payload pairs instead. This would also simplify user code. Does it make sense for you?

jakepusateri commented 8 years ago

That sounds good. A rootContainer prop maybe called preloadedQueries that takes an array of { relayQuery, data} objects would let the client code not mess with getStoreData and handleQueryPayload or have to pass an environment. The data array returned from prepareData would then also be able to include these queries instead of having to concat them in user code-side.

Should forceFetch stay an option, or would it be implied by preloadedQueries?

denvned commented 8 years ago

I think we don't need the forceFetch option, but we need to replace the forceFetch call with primeCache inside.

denvned commented 8 years ago

@jakepusateri Do you have any plans on updating your PR accordingly?

jakepusateri commented 8 years ago

Yeah I'll update it tomorrow.

jakepusateri commented 8 years ago

Updated #51