denvned / isomorphic-relay

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

Allow for one off query overrides #43

Closed toddtarsi closed 8 years ago

toddtarsi commented 8 years ago

Hello, my name is Todd and I'm a huge fan of your codebase! An issue I'm encountering though is due to how the current solution for serverside rendering requires modifying the network global via injectNetworkLayer, so I'm pitching this change to the relay team: https://github.com/facebook/relay/pull/1290

Instead of changing our serverside rendering context via injectNetworkLayer and routing queries through a FIFO system, I'd like to pass an overrides object as an additional argument, where fields are provided to modify my request as needed in the network layer. Please weigh in as you feel in both this thread and that one, as I am still getting comfortable with Relay. If this works for everyone though, this should be all that's needed to make server side rendering much cleaner.

robrichard commented 8 years ago

@toddtarsi what are you modifying in the requests in your override? Could you instead make a new network layer for each request?

This is how you could do it if you needed to set unique auth info

app.get('/my-route', function(req, res) {
  const networkLayer = new Relay.DefaultNetworkLayer('https://example.com/graphql', {
    headers: {
      Authorization: req.get('Authorization')
    }
  });

   IsomorphicRelay.prepareData(rootContainerProps, networkLayer).then({data, props} => {
        ...
   })
})
toddtarsi commented 8 years ago

You can do it that way, but I really worry about that. Doesn't that call injectNetworkLayer with the sensitive information under the covers, which breaks concurrency?

From this thread https://github.com/denvned/isomorphic-relay-router/issues/13 I got the impression that this code requires wrapping the query processing system in a FIFO system for serverside rendering, which is not good in nodejs. By allowing for customizing the sendQueries call, we can avoid polluting the global scope with sensitive context information.

On Tuesday, July 19, 2016, Rob Richard notifications@github.com wrote:

@toddtarsi https://github.com/toddtarsi what are you modifying in the requests in your override? Could you instead make a new network layer for each request?

This is how you could do it if you needed to set unique auth info

app.get('/my-route', function(req, res) { const networkLayer = new Relay.DefaultNetworkLayer('https://example.com/graphql', { headers: { Authorization: req.get('Authorization') } });

IsomorphicRelay.prepareData(rootContainerProps, networkLayer).then({data, props} => { ... }) })

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/denvned/isomorphic-relay/pull/43#issuecomment-233595065, or mute the thread https://github.com/notifications/unsubscribe-auth/AEfUsVclkFLffGmuatZLAyPD68KD9pZlks5qXKmhgaJpZM4JPTf0 .

robrichard commented 8 years ago

I could be wrong, but I think it's not an issue since relay 0.8.0. Prior to that, there was only one global Relay.Store. Now you can make a new Relay.Environment for each request, each with its own networkLayer. That's exactly what the current version of isomorphic-relay does (https://github.com/denvned/isomorphic-relay/blob/master/src/prepareData.js#L10). It's calling injectNetworkLayer on the custom environment, not the global Relay.injectNetworkLayer.

toddtarsi commented 8 years ago

Oh, awesome! Thank you very much for taking the time to explain that. I am still getting my feet wet with relay. That is a huge relief!

On Jul 19, 2016, at 8:24 AM, Rob Richard notifications@github.com wrote:

I could be wrong, but I think it's not an issue since relay 0.8.0 https://github.com/facebook/relay/releases/tag/v0.8.0. Prior to that, there was only one global Relay.Store. Now you can make a new Relay.Environment for each request, each with its own networkLayer. That's exactly what the current version of isomorphic-relay does (https://github.com/denvned/isomorphic-relay/blob/master/src/prepareData.js#L10 https://github.com/denvned/isomorphic-relay/blob/master/src/prepareData.js#L10). It's calling injectNetworkLayer on the custom environment, not the global Relay.injectNetworkLayer.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/denvned/isomorphic-relay/pull/43#issuecomment-233631006, or mute the thread https://github.com/notifications/unsubscribe-auth/AEfUsX7YhmaMJH5rJCaQAPWIxJrxB6u-ks5qXNAOgaJpZM4JPTf0.

toddtarsi commented 8 years ago

My use case is satisfied, and if you'd like, I can close this pull. Thanks!

toddtarsi commented 8 years ago

Closing this, thank you again @robrichard