-
Notifications
You must be signed in to change notification settings - Fork 37
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
React Router 7 integration #394
Conversation
commit: |
#379 Bundle Size — 1.32MiB (+5.83%).561c16e(current) vs c125a08 main#352(baseline) Note Bundle removed 1 duplicate package – View changed duplicate packages Bundle metrics
|
Current #379 |
Baseline #352 |
|
---|---|---|
Initial JS | 1.04MiB (+4.22% ) |
1016.91KiB |
Initial CSS | 70B |
70B |
Cache Invalidation | 85.18% |
24.97% |
Chunks | 36 (+5.88% ) |
34 |
Assets | 62 (+5.08% ) |
59 |
Modules | 678 (+6.6% ) |
636 |
Duplicate Modules | 119 (+12.26% ) |
106 |
Duplicate Code | 5.37% (+15.24% ) |
4.66% |
Packages | 25 (-3.85% ) |
26 |
Duplicate Packages | 0 (-100% ) |
1 |
Bundle size by type 2 changes
2 regressions
Current #379 |
Baseline #352 |
|
---|---|---|
JS | 1.31MiB (+5.8% ) |
1.24MiB |
Other | 10.01KiB (+10.12% ) |
9.09KiB |
CSS | 70B |
70B |
Bundle analysis report Branch pr/react-router-7 Project dashboard
Generated by RelativeCI Documentation Report issue
to prevent type mismatches
updated the initial comment, ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple questions but otherwise looks great!
? (new IncrementalSchemaLink({ schema }) as any as ApolloLink) | ||
: new HttpLink({ uri: "/graphql" }); | ||
|
||
export const makeClient = (request?: Request) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export const makeClient = (request?: Request) => { | |
export const makeClient = () => { |
Is this argument needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to showcase that the argument is there in case someone wanted to access a header on the server or something.
|
||
const ABORT_DELAY = 5_000; | ||
|
||
export default function handleRequest( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, is this code we expect our end users to write themselves?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file gets created when running yarn react-router reveal
.
The diff we expect our users to write is this:
diff --git a/integration-test/react-router/app/entry.client.tsx b/integration-test/react-router/app/entry.client.tsx
index 08ab4ec..4f23189 100644
--- a/integration-test/react-router/app/entry.client.tsx
+++ b/integration-test/react-router/app/entry.client.tsx
@@ -1,12 +1,17 @@
import { startTransition, StrictMode } from "react";
import { hydrateRoot } from "react-dom/client";
import { HydratedRouter } from "react-router/dom";
+import { makeClient } from "./apollo";
+import { ApolloProvider } from "@apollo/client/index.js";
startTransition(() => {
+ const client = makeClient();
hydrateRoot(
document,
<StrictMode>
- <HydratedRouter />
+ <ApolloProvider client={client}>
+ <HydratedRouter />
+ </ApolloProvider>
</StrictMode>
);
});
diff --git a/integration-test/react-router/app/entry.server.tsx b/integration-test/react-router/app/entry.server.tsx
index bffa8aa..a4bf85e 100644
--- a/integration-test/react-router/app/entry.server.tsx
+++ b/integration-test/react-router/app/entry.server.tsx
@@ -6,6 +6,8 @@ import { ServerRouter } from "react-router";
import { isbot } from "isbot";
import type { RenderToPipeableStreamOptions } from "react-dom/server";
import { renderToPipeableStream } from "react-dom/server";
+import { makeClient } from "./apollo";
+import { ApolloProvider } from "@apollo/client/index.js";
const ABORT_DELAY = 5_000;
@@ -27,12 +29,15 @@ export default function handleRequest(
? "onAllReady"
: "onShellReady";
+ const client = makeClient(request);
const { pipe, abort } = renderToPipeableStream(
- <ServerRouter
- context={routerContext}
- url={request.url}
- abortDelay={ABORT_DELAY}
- />,
+ <ApolloProvider client={client}>
+ <ServerRouter
+ context={routerContext}
+ url={request.url}
+ abortDelay={ABORT_DELAY}
+ />
+ </ApolloProvider>,
{
[readyOption]() {
shellRendered = true;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which is actually a good start for writing a README :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah gotcha! That makes sense. Thanks for the context! I haven't tried out anything with RR 7 yet so I'm a bit naive on whats there 😆
From my side, this works now and I could even circumvent the need for a PR to
turbo-stream
(although it would be nice if that would have native support forReadableStream
).There is one outstanding type PR for remix-run/react-router#12264, but I think we can already get this reviewed at this point.