Skip to content
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

[WIP] Possible evolution of the Relay integration with Next.js #269

Merged
merged 3 commits into from
Jan 11, 2023

Conversation

alunyov
Copy link
Contributor

@alunyov alunyov commented Jan 11, 2023

Note: This is WIP and not ready to be merged. I’m sharing for feedback and adjustments on the direction.

Summary:

  • Data-fetching for routes is happening in the Relay Server Components, using fetchQuery this version is different from what we export from relay-runtime - (maybe we’ll need a different name). Internally, it fetches the query, publish to the relay store. This fetchQuery can only be called on the server.

  • RelayRoot - RSC, simple wrapper, that passes serialized list of fetched queries to the RelayClientRoot

  • RelayClientRoot - client component that wraps the children with Relay context provided, and publishes the fetched server queries to the client store, so the state of the local store is matching the one on the server.

    • Everything under this RelayClientRoot is normal client-first relay with hooks and things... But there is no root query component (with usePreloadedQuery or similar...)
  • Instead of useFragment on the server we can use readFragmentData function - this will allow for individual server components define own data requirements, and the view will be fully server-rendered, with possible client leafs. The fragments composed into a query and fetched in the top RSC with fetchQuery.


So, this is a “sketch” of the possible integration, so many things may not work:

  • Preserving the state of the store, when navigating between the list and the issue view
  • Multiple leafs with RelayClientRoot

Not sure if it’s working:

  • useLazyLoadQuery - under RelayClientRoot.
  • 3D/queryPreloading
  • stream/defer

@alunyov
Copy link
Contributor Author

alunyov commented Jan 11, 2023

Also, worth noting: this version publish of the same query payload 3 times (maybe more): two times on the server, for SC and under each RelayRoot. And then on the client, under RelayClientRoot.

Copy link
Contributor

@alvarlagerlof alvarlagerlof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Haven't tested locally yet, but the user-facing apis look nice, and are definitely more in line with what a server components implementation could do. Was attempting something like this myself but got stuck not knowing the relay apis well enough to piece it all together.

I looked at the code and found a few nitpicks and questions. But will take another look again after running the code.

Comment on lines +13 to +14
const environment = useMemo(() => {
const environment = createEnvironment();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A little confusing imo that these are named the same thing.

export default function RelayClientRoot(props: {
children: React.ReactNode;
fetchedQueries: readonly FetchRecord[];
}): React.ReactElement {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the return type here help in any way?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really, it just a muscle-memory from working with flow (where every public export has to be explicitly typed - args/return type).

Comment on lines +48 to +56
function createFetchRecord(
operationDescriptor: OperationDescriptor,
response: GraphQLSingularResponse
): FetchRecord {
return {
operationDescriptor,
response,
} as FetchRecord;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why have this as a function? It looks like it only takes two inputs and puts them in an object. If you just add them as an object to the fetchRecords: FetchRecord[] directly you'd get the same kind of type errors (if that is the goal).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to simplify this part. Initial version were exposed this type to the user (and I was trying to make it opaque). We don't need this now, I think

<div className={styles.main}>
<MainViewClientComponent preloadedQuery={preloadedQuery} />
</div>
<Suspense fallback="Loading...">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should likely be a loading.tsx instead since that will show up earlier than the suspense fallback for pages.

https://beta.nextjs.org/docs/routing/loading-ui. The "Good to know" section:

"Navigation is immediate, even with server-centric routing."

{props.children}
</RelayClientRoot>
);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's possible to init this in layout.tsx, but without data, and then have another client component for each page that just adds the queried data "top root" one.

That could allow keeping data around while navigating, but ofc doesn't stop the server components from refetching data that is already there.

@sibelius
Copy link
Contributor

Can we find a way to use the same function “useFragment” on both client and server?

@alunyov
Copy link
Contributor Author

alunyov commented Jan 11, 2023

Can we find a way to use the same function “useFragment” on both client and server?

Well, since readFragmentData this is not part of the core framework, you can name it useFragment, but for the sake of example, and to be explicit (and avoid possible confusion) I named it differently.

@a-marcel
Copy link

a-marcel commented Jan 11, 2023 via email

@alunyov alunyov merged commit 29ed730 into relayjs:main Jan 11, 2023
@alunyov
Copy link
Contributor Author

alunyov commented Jan 11, 2023

Ugh... Accidentally pushed this to the main branch. Already reverted, I'll resubmit PR.

@alunyov
Copy link
Contributor Author

alunyov commented Jan 11, 2023

Re-opened as #270

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants