Bug: Cache in SSR

[HsuTing]

I found requestCache caused a bug.
In server side, componentDidUpdate never be called. As a result, requestCache never be cleaned.

I used react-relay-network-modern-ssr in my project and this was ok when I loaded the website for the first time. This queried data correctly. However, after I reload the website, this did not request again to get the newest data.

Because nodejs never import each module again, requestCache will never be cleaned in server.

[jstejada]

hey @HsuTing , which version are you using?

[HsuTing]

@jstejada
react-relay@^1.7.0
Do you need the other version of the packages?

[sibelius]

Can you try with 2.0.0-rc1?

[HsuTing]

Same problem

[sibelius]

@HsuTing can you try to come up with a failing test ?

[robrichard]

Could this request cache also potentially leak data to unauthorized users? If the same query is issued with different environments with different authorization levels, the user with less authorization could potentially get the cached response from the user with higher authorization?

[HsuTing]

@sibelius OK, I will try to make a simple example.
@robrichard Yes. Users can not get the right data after they login. The data is locked by the cache response.

[HsuTing]

@sibelius
Here is a simple example.
If you need the more information, let me know 😸 .

[crash7]

Hi all! I can confirm the issue too with a different implementation of Relay SSR than the used in @HsuTing example.

The real problem is that the requestCache[key] is only cleared in componentDidMount, which never happens on SSR:
https://github.com/facebook/relay/blob/master/packages/react-relay/modern/ReactRelayQueryRenderer.js#L189-L194

Because of that, the "request" is always in flight: https://github.com/facebook/relay/blob/master/packages/react-relay/modern/ReactRelayQueryRenderer.js#L365

Would be nice to have a way to force that fetch someway, maybe with an extra prop in the QueryRender component that I can set when process.browser is false.

[m64253]

I think having "state" like this outside of the component instance is always going to an issue server side?

Maybe this can be moved to the network?

Would love to help to resolve this but I'm a bit uncertain on what the actual problem being solved by requestCache is, simultaneous request duplication?

[edvinerikson]

Just speculating..

Looking at the code, this seem to try fixing using side-effects inside a method (constructor) which should not have such side-effects. I believe the problem attempted to solve occurs when you run the code in strictmode in development.

It really seem to be a non-problem (double call of constructor) in production but may perhaps look odd in development, I don't know if anyone at FB can confirm this or not? @flarnie, perhaps?

By design this implementation is correct as it tries to de-dupe requests made by different react component instances, though I am not sure it's a feasible optimisation to do given the effects it has in server environments. For the record, there is a similar bug like this existing today in the React Context implementation as well (facebook/react#13874).

One solution to this may be to clear the cache in getDerivedStateFromProps. It's executed immediately after the constructor on the server.. Its not supported but the "side-effects free methods" ship has already sailed so..

Still to me this sounds like a fix to a problem that has far more significant side-effects than what it solves currently (given the context from a outside contributor.. perhaps more details is provided internally).

Maybe someone from React team can help out suggesting a better way here?
CC @gaearon @sebmarkbage

Edit

As @m64253 says, perhaps this can be solved on a application level by offering de-duping in the network layer provided by app developers. It may not be a great DX when using strictmode but at least it doesn't affect production runtimes.

[crash7]

Now that @flarnie left facebook[1], who will help us? 😱 😱

[1] https://twitter.com/ProvablyFlarnie/status/1081365351139467266

[josephsavona]

We’re actively working on suspense-compatible APIs, which will include addressing issues such as these. Stay tuned!

[holmesal]

We've been struggling with this as well.

The only way we've been able to ensure that the query is actually fetched under SSR is to include a nonce variable (variables are used as part of the cache key) - but this creates a memory leak, as the cache grows with every request and is never cleared.

@josephsavona any progress that could help us out? We're planning to fork ReactRelayQueryRenderer to fix our immediate issue, but would be good to have a proper fix on the Relay side!

[sibelius]

this is fixed on relay-hooks relay-tools/relay-hooks#7

you can try that for now