-
Notifications
You must be signed in to change notification settings - Fork 32
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
0.9.0 (3) - add integration test with experimentally patched React version #219
Conversation
"version": "0.0.0", | ||
"type": "module", | ||
"scripts": { | ||
"prepare": "rm -rf node_modules/@apollo/client-react-streaming; mkdir node_modules/@apollo/client-react-streaming && cp -r ../../packages/client-react-streaming/{package.json,dist} node_modules/@apollo/client-react-streaming", |
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 is necessary so it doesn't pick up the react
folder from packages/client-react-streaming/node_modules
and mixes two react versions.
import type { DataTransportProviderImplementation } from "@apollo/client-react-streaming"; | ||
import { DataTransportContext } from "@apollo/client-react-streaming"; | ||
import { useMemo, useActionChannel, useStaticValue, useRef } from "react"; | ||
|
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.
Note to self: double check the approach of queueing a single promise for request & response
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.
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.
My comments were petty 😆 , otherwise looks great! 🎉
let vite; | ||
let bootstrapModules = []; | ||
let assets = []; | ||
console.log({ isProduction }); |
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.
console.log({ isProduction }); |
Not sure if this was meant to stay or not 🙂
if (!isProduction) { | ||
const { createServer } = await import("vite"); | ||
vite = await createServer({ | ||
server: { middlewareMode: true, hmr: true }, | ||
appType: "custom", | ||
base, | ||
}); | ||
app.use(vite.middlewares); | ||
} else { | ||
const compression = (await import("compression")).default; | ||
const sirv = (await import("sirv")).default; | ||
app.use(compression()); | ||
app.use(base, sirv("./dist/client", { extensions: [] })); | ||
const index = await readFile("./dist/client/index.html", "utf-8"); | ||
for (const script of index.matchAll( | ||
/<script type="module" \w+ src="(.*)">/g | ||
)) { | ||
bootstrapModules.push(script[1]); | ||
} | ||
for (const link of index.matchAll( | ||
/<link rel="stylesheet" \w+ href="(.*)">/g | ||
)) { | ||
assets.push(link[1]); | ||
} | ||
} |
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 know this is meant to be more of a "demo" (ish), but any chance we could flip the conditional here? I always find it easier with if/else
to use the positive connotation first.
Rather than:
if (!isProduction) {
// other envs
} else {
// production builds
}
Use this:
if (isProduction) {
// production stuff
} else {
// other environments
}
This keeps the "production" configuration closer to the conditional test for it, rather than breaking it apart.
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 is based on the Vite React SSR template and I'm trying to keep the diff as small as possible, so while I agree and would really like to swap it, it's probably better we don't :)
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 didn't realize these were copy-pasted so makes sense! Thanks for pointing me to that.
res.send("<!doctype><p>Error</p>"); | ||
}, | ||
onError(x) { | ||
console.log("Error", x); |
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.
console.log("Error", x); |
Since you already have a console.error
below, could we just use that?
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.
Uh yeah, I'm generally gonna remove a bunch of these logs :)
This adds an integration test for the "experimental React hooks" solution and moves the
@apollo/client-react-streaming/experimental-react-transport
out of the package, only into the test.It's not overly complicated, and we probably don't need to really ship it as part of a package.