relay-nextjs icon indicating copy to clipboard operation
relay-nextjs copied to clipboard

Problem with 2.0 migration

Open FINDarkside opened this issue 1 year ago • 8 comments

I didn't test if the example migration works, but I simply cannot get 2.0.1 to work unless I remove this line in my project: https://github.com/RevereCRE/relay-nextjs/blob/9cb7898273774352b18766903d1ae8b1d6566e98/example/src/lib/relay_client_environment.ts#L25

Otherwise I'll only get this error: TypeError: Cannot read properties of null (reading 'commitPayload'). Screenshot from 2023-08-01 18-17-37

If you have any ideas please let me know. I don't have a repro right now but I'll look into making one if you think there's some problem with just deleting that line.

FINDarkside avatar Aug 01 '23 15:08 FINDarkside

Removing that line removes the safeguard around creating a new client environment on the server, so it looks like the server environment isn't being initialized correctly, or the page isn't passing along the preloaded query correctly. Can you paste the withRelay function call?

rrdelaney avatar Aug 01 '23 17:08 rrdelaney

We have utility function we use to add the withRelay, translations etc, but even when I reduced it to this:

export default withRelay(HomePage, IndexQuery, {
  createClientEnvironment: () => getClientEnvironment()!,
  createServerEnvironment: async (
    ctx,
  ) => {
    const { createServerEnvironment } = await import('src/util/relay/createServerEnvironment');
    return createServerEnvironment(ctx.req,ctx.res,'en');
  },
});

It's still the same error. I'll look a bit more into this when I have time. Something to note is that createServerEnvironment does return a RelayModernEnvironment and the data is fetched correctly and is available in the pageProps. Also, the error happens during SSR, not client side. Obviously could also happen in client side as well, I just haven't gotten that far yet :smile:

FINDarkside avatar Aug 02 '23 12:08 FINDarkside

Okay I finally found the problem. pageProps has preloadedQuery field which is not enumerable meaning that it's lost when we do {...pageProps} in one of our _app.tsx wrapper components. This behaviour has changed in v2 since it wasn't a problem earlier :thinking:

Not sure if it's the only problem I have as adding our proper withRelay util call back I'm back at the same error. I'll give up for now, might look into it later :sweat_smile:

FINDarkside avatar Aug 02 '23 13:08 FINDarkside

Okay one more update. I finally managed to get rid of the error by finding all the places where I expanded pageProps and manually copying preloadedQuery to the new props object. But then I ran into the next error on client side: TypeError: environment.retain is not a function

FINDarkside avatar Aug 02 '23 14:08 FINDarkside

@FINDarkside Any more updates? Any way we can help?

rrdelaney avatar Aug 15 '23 20:08 rrdelaney

I'll still have to get back to updating it probably this week. I assume my current client side issues are possibly caused by the same thing. Assuming that's the case then the only real "issue" would be that preloadedQuery and possibly some other fields are non-enumerable. It's a bit unexpected since it's common for people to do stuff like {...props} down the line, which in this case would lose preloadedQuery and possibly some other fields. Is there some reason why these fields must be non-enumerable?

FINDarkside avatar Aug 16 '23 07:08 FINDarkside

Is there some reason why these fields must be non-enumerable?

The field are non-enumerable to prevent Next from attempting to serialize data isn't supposed to. It lets us initialize a server-only store, get the data out, and carefully control what Next actually sends over in initial props. I might be possible to workaround it in another way, but would be a large refactor.

rrdelaney avatar Aug 16 '23 16:08 rrdelaney

Unfortunately we have the same issue and therefore we can't upgrade from 1.x

However for us there might be another way to get it to work: we are able to remove all the _app customization that causes the same errors @FINDarkside ran into.

But if we do that, it seems Next.js tries to do Automatic Static Optimization and calls the page query at compile time, which fails because our build system isn't designed to have the backend available during compile time and our pages aren't meant to be statically generated at compile time anyway.

I tried turning this behavior off but I found no way to force it off. If there was a way, maybe we could get past the environment.retain is not a function error.

fazo96 avatar Oct 02 '23 08:10 fazo96