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

add onStreamedValue router event when a start-SSRed value arrived #2698

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

phryneas
Copy link
Contributor

@phryneas phryneas commented Nov 4, 2024

You can see this in action here:

https://apolloclient-integration-tanstack-start-kftqizzwm.vercel.app/useSuspenseQuery

Essentially, we need to be notified when a value from the server arrives, as our core is written with a "stream of individual events" type approach in mind.

The integration package PR can be found here: apollographql/apollo-client-nextjs#412 and you can find the code for the above page here: https://github.com/apollographql/apollo-client-nextjs/blob/pr/tanstack-start/integration-test/tanstack-start/app/routes/useSuspenseQuery.tsx

And here's the README for the integration package.

@phryneas phryneas changed the title exploration for Apollo Client integration add onStreamedValue router event when a start-SSRed value arrived Jan 13, 2025
@phryneas phryneas marked this pull request as ready for review January 13, 2025 11:57
@@ -126,7 +126,17 @@ export const useMetaElements = () => {
// children={`
// __TSR__ = {
// matches: [],
// streamedValues: {},
// streamedValues: new Proxy(
Copy link
Contributor

Choose a reason for hiding this comment

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

is there as solution that does not need a proxy?

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 while keeping this a "plain object", as afaik there is no other way of detecting a property is added.

If the injected script tag would so something like __TSR__.streamedValues.push([name, value]) on an array, or __TSR__.streamedValues.set(name, value) on a Map, those .push or .set properties could be overwritten to also call __TSR__ROUTER__.emit - but for a plain object property assignment the only way to do this is a Proxy.

For an "array-type" implementation, see https://github.com/apollographql/apollo-client-nextjs/blob/main/packages/client-react-streaming/src/ManualDataTransport/lateInitializingQueue.ts as an example - it is just created as an array and later registerLateInitializingQueue is called, consuming backpressure once and passing newly incoming data directly into the callback function.
Of course that's a very different approach and I didn't want to change too much, so I went with this for now.

@phryneas
Copy link
Contributor Author

As requested in a chat with @schiller-manuel, here the reworked version.

__TSR__.streamedValues now is no longer a growing object of type Record<string, { value: any, parsed: any }>/

Instead, it starts off as an array of type Array<[key: string, value: any]>.

Once React router initializes, it consumes all values that have collected so far as backpressure and then replaces the array's .push function with a function that consumes incoming values immediately.

Those values are now stored on router.streamedValues in the old format. Every time, after a value has been added there, a onStreamedValue event with the key will be emitted by the router - that allows other libraries to listen for incoming streamed values.

this.injectScript(
`__TSR__.streamedValues['${key}'] = { value: ${this.serializer?.(this.options.transformer.stringify(value))}}`,
`__TSR__.streamedValues.push([${this.serializer?.(key)}, ${this.serializer?.(this.options.transformer.stringify(value))}])`,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

key was not serialized before, so keys containing ' could actually cause JavaScript errors or code injection.

phryneas added a commit to apollographql/apollo-client-nextjs that referenced this pull request Jan 20, 2025
@phryneas
Copy link
Contributor Author

This is now deployed at https://apolloclient-integration-tanstack-start-b3k74ebxl.vercel.app/useSuspenseQuery so you can poke it there if you want to.

Copy link

nx-cloud bot commented Jan 20, 2025

View your CI Pipeline Execution ↗ for commit f82f00f.

Command Status Duration Result
nx affected --targets=test:eslint,test:unit,tes... ✅ Succeeded 5m 43s View ↗
nx run-many --target=build --exclude=examples/*... ✅ Succeeded 1m 19s View ↗

☁️ Nx Cloud last updated this comment at 2025-01-27 14:26:53 UTC

Copy link

pkg-pr-new bot commented Jan 20, 2025

Open in Stackblitz

More templates

@tanstack/arktype-adapter

npm i https://pkg.pr.new/@tanstack/arktype-adapter@2698

@tanstack/create-router

npm i https://pkg.pr.new/@tanstack/create-router@2698

@tanstack/directive-functions-plugin

npm i https://pkg.pr.new/@tanstack/directive-functions-plugin@2698

@tanstack/eslint-plugin-router

npm i https://pkg.pr.new/@tanstack/eslint-plugin-router@2698

@tanstack/create-start

npm i https://pkg.pr.new/@tanstack/create-start@2698

@tanstack/history

npm i https://pkg.pr.new/@tanstack/history@2698

@tanstack/react-cross-context

npm i https://pkg.pr.new/@tanstack/react-cross-context@2698

@tanstack/react-router

npm i https://pkg.pr.new/@tanstack/react-router@2698

@tanstack/react-router-with-query

npm i https://pkg.pr.new/@tanstack/react-router-with-query@2698

@tanstack/router-cli

npm i https://pkg.pr.new/@tanstack/router-cli@2698

@tanstack/router-core

npm i https://pkg.pr.new/@tanstack/router-core@2698

@tanstack/router-devtools

npm i https://pkg.pr.new/@tanstack/router-devtools@2698

@tanstack/router-generator

npm i https://pkg.pr.new/@tanstack/router-generator@2698

@tanstack/router-plugin

npm i https://pkg.pr.new/@tanstack/router-plugin@2698

@tanstack/router-vite-plugin

npm i https://pkg.pr.new/@tanstack/router-vite-plugin@2698

@tanstack/server-functions-plugin

npm i https://pkg.pr.new/@tanstack/server-functions-plugin@2698

@tanstack/start

npm i https://pkg.pr.new/@tanstack/start@2698

@tanstack/start-api-routes

npm i https://pkg.pr.new/@tanstack/start-api-routes@2698

@tanstack/start-client

npm i https://pkg.pr.new/@tanstack/start-client@2698

@tanstack/start-config

npm i https://pkg.pr.new/@tanstack/start-config@2698

@tanstack/start-plugin

npm i https://pkg.pr.new/@tanstack/start-plugin@2698

@tanstack/start-router-manifest

npm i https://pkg.pr.new/@tanstack/start-router-manifest@2698

@tanstack/start-server

npm i https://pkg.pr.new/@tanstack/start-server@2698

@tanstack/start-server-functions-client

npm i https://pkg.pr.new/@tanstack/start-server-functions-client@2698

@tanstack/start-server-functions-fetcher

npm i https://pkg.pr.new/@tanstack/start-server-functions-fetcher@2698

@tanstack/start-server-functions-handler

npm i https://pkg.pr.new/@tanstack/start-server-functions-handler@2698

@tanstack/start-server-functions-server

npm i https://pkg.pr.new/@tanstack/start-server-functions-server@2698

@tanstack/start-server-functions-ssr

npm i https://pkg.pr.new/@tanstack/start-server-functions-ssr@2698

@tanstack/virtual-file-routes

npm i https://pkg.pr.new/@tanstack/virtual-file-routes@2698

@tanstack/valibot-adapter

npm i https://pkg.pr.new/@tanstack/valibot-adapter@2698

@tanstack/zod-adapter

npm i https://pkg.pr.new/@tanstack/zod-adapter@2698

commit: f82f00f

@phryneas
Copy link
Contributor Author

Reworked as requested :)

@phryneas
Copy link
Contributor Author

phryneas added a commit to apollographql/apollo-client-nextjs that referenced this pull request Jan 23, 2025
* add `(TeeTo|ReadFrom)ReadableStreamLink` links

* add `skipDataTransport` function

* also export type `ReadableStreamLinkEvent`

* forgot file

* implement multipart streaming

* cleanup

* WIP

* handle re-consumption of readFromReadableStreamKey

* close on unsubscribe, catch errors

* tryClose, test adjustments

* adjust errorlink type

* streamline bundling

* fix jest integration test

* PreloadQuery: add `@defer` example

* add detail, suspense-transition-bug

* work around bug for now

* exports

* add exported types

* initialize basic react-router project

* clean up and `yarn react-router reveal`

* add raw experiment

* move eslint config into shared position

* Fix bundling configuration so parts of the `graphql` package don't end (#388)

* Release version 0.11.6@latest to npm

* Update peer deps for React 19 compatibility (#399)

* update devDependencies to React 19, adjust tests and examples (#400)

* Release version 0.11.7@latest to npm

* add example

* update react-router

* working

* patch PR in

* last adjustments

* fix up test for new deps

* clean up template fluff

* move `IncrementalSchemaLink` and others to `shared`

* use `@defer` in the example

* update turbo-stream patch

* update example to React 19

* adjust `prepublishOnly`

* add to pkg-pr-new-publish

* add empty unit test

* increase delay

* streamline integration test build

* needs more time in CI

* exclude shared

* copy react-router package to tanstack-start folder

* package init

* init tanstack start project

* current progress

* fix duplicate `@apollo/client` dependency

* make things work

* adjustments

* generalize apiRoute

* api, use schemalink on server

* update tanstack start

* add TSR devtools

* workaround for TanStack/router#3117

* trigger CI

* update patchfiles

* add new properties

* changeset

* trigger CI

* fix build warning

* port over vercel template from https://github.com/remix-run/react-router-templates/tree/main/vercel

* adjust folder structure

* more vercel adjustments

* use `promiscade` instead of a patched `turbo-stream` package

* update promiscade

* add promiscade-related comment

* lockfile

* workarounds for promiscade

* move `graphql` dependency into monorepo
to prevent type mismatches

* adjust comments

* these types are correct now

* update lockfile

* rename route

* add `useSuspenseQuery` demo, too

* fix import

* simplify client-side

* remove log

* remove unneccessary type assertion

* slight changes, README

* fix up important block

* fix typo, reorder

* fixup shape test

* add bundleInfo, adjust tests

* adjust tests

* adjust for old node

* add globalThis.window in test

* bump promiscade

* update tanstack start, remove workaround for fixed bug

* update package.json

* add TanStack/router#3161

* lockfile

* update correct react types

* eliminate context reliance

this makes it easier to use `yalc`

* sync updates from TanStack/router#2698

* lockfiles

* script

* fighting with duplicate dependencies

* unify react types version

* remove unused path

* more config file mentions

* TSR 6d6780b255c458e9776f15c134d93fab0aeafa80

* pass `request` to `makeClient` in entry.server.tsx

* delete file after merge

* lockfile

* Update packages/tanstack-start/README.md

Co-authored-by: Jerel Miller <[email protected]>

---------

Co-authored-by: phryneas <[email protected]>
Co-authored-by: Nick Muller <[email protected]>
Co-authored-by: Jerel Miller <[email protected]>
@phryneas
Copy link
Contributor Author

And reworked again so it works with the changed streaming :)

const __TSR_SSR__: StartSsrGlobal = {
matches: [],
streamedValues: {},
setStreamedValue(key, value) {
__TSR_SSR__.streamedValues[key] = { value }
if (typeof __TSR_ROUTER__ !== 'undefined') {
Copy link
Contributor

Choose a reason for hiding this comment

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

there might a race condition where a value is streamed but router is not hydrated yet.
streamedKeys is initialized with all previously streamed keys, so that won't be affected.

remind me again please, why don't we just call __TSR_ROUTER__.clientSsr!.streamedKeys.add(key) here? why do we need that event emitter pattern?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the event is the thing I am really interested in.

Our library is listening for these incoming events.
It only uses streamedKeys on initialization to catch up with previously streamed values to consume back pressure, but it also needs everything happening in the future when it happens.

Of course we could emote the event and call .add here, but to me it seemed more intuitive to write to streamedKeys only in one place, not in two.

Copy link
Contributor Author

@phryneas phryneas Jan 27, 2025

Choose a reason for hiding this comment

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

https://github.com/apollographql/apollo-client-nextjs/blob/1306572238ff9d9421b16164fc039d5b61f17ce9/packages/tanstack-start/src/ApolloProvider.tsx#L79-L85

We stream events like "query started", "query received data", "query finished" etc. from SSR to the browser by calling router.streamValue (still have to push the change to the new call here).

https://github.com/apollographql/apollo-client-nextjs/blob/1306572238ff9d9421b16164fc039d5b61f17ce9/packages/tanstack-start/src/ApolloProvider.tsx#L57-L68

We do this as these events could happen in a waterfalled nested suspense boundary, so they wouldn't necessarily be available at the time router.options.dehydrate runs - even the start of a query might happen much later, and multiple values might be streamed in for a single query with a multipart response.

I'm honestly a bit surprised that the React Query implementation only collects values at the time of router.options.dehydrate, I would assume it would also need an approach like this - could that be a bug in the RQ integration?

Copy link
Contributor

Choose a reason for hiding this comment

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

it does not only collect values at time of router.options.dehydrate, it also collects values after a query finished.

Copy link
Contributor Author

@phryneas phryneas Jan 27, 2025

Choose a reason for hiding this comment

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

But does it collect new queries that start after router.options.dehydrate and when no query was running?
I might be missing a puzzle piece here :)

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

Successfully merging this pull request may close these issues.

2 participants