Skip to content

fix(start): discard stream controller errors when closed #3161

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

Merged
merged 2 commits into from
Jan 21, 2025

Conversation

phryneas
Copy link
Contributor

@phryneas phryneas commented Jan 14, 2025

Stream Controller: try enqueue/close without throwing error

Both controller.enqueue and controller.close throw an error if the stream has already been closed.

This could be the case because the reading side just doesn't have interest in more incoming values and has closed the stream on the reading side.

Up until now, this would cause an error to be thrown on the console once more chunks or a close request were streamed in.

This wraps .close and .enqueue in a try..catch block and disregards the result.

Reading the standard I don't see any possible other reasons for an error to be thrown, so this should be fine.

phryneas added a commit to apollographql/apollo-client-integrations that referenced this pull request Jan 14, 2025
Copy link

nx-cloud bot commented Jan 15, 2025

View your CI Pipeline Execution ↗ for commit 7bb69a8.

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

☁️ Nx Cloud last updated this comment at 2025-01-20 18:49:45 UTC

Copy link

pkg-pr-new bot commented Jan 15, 2025

Open in Stackblitz

More templates

@tanstack/arktype-adapter

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

@tanstack/create-router

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

@tanstack/directive-functions-plugin

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

@tanstack/eslint-plugin-router

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

@tanstack/create-start

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

@tanstack/history

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

@tanstack/react-cross-context

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

@tanstack/react-router

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

@tanstack/react-router-with-query

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

@tanstack/router-cli

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

@tanstack/router-devtools

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

@tanstack/router-generator

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

@tanstack/router-plugin

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

@tanstack/router-vite-plugin

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

@tanstack/server-functions-plugin

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

@tanstack/start

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

@tanstack/start-plugin

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

@tanstack/valibot-adapter

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

@tanstack/virtual-file-routes

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

@tanstack/zod-adapter

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

commit: 7bb69a8

@SeanCassiere SeanCassiere changed the title Stream Controller: try enqueue/close without throwing error fix(start): discard steam controller errors when closed Jan 15, 2025
Comment on lines 52 to 55
controller = {
enqueue: (chunk: unknown) => { try { c.enqueue(chunk); } catch {} },
close: () => { try { c.close(); } catch {} },
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doing this here seems to be the place that is the most defensive - anywhere "further out", it could swallow errors e.g. if controller is still undefined etc.

@phryneas
Copy link
Contributor Author

Force-pushed to reflect the latest change with tsrScript.ts

@schiller-manuel schiller-manuel changed the title fix(start): discard steam controller errors when closed fix(start): discard stream controller errors when closed Jan 20, 2025
@schiller-manuel schiller-manuel merged commit 76fd97e into TanStack:main Jan 21, 2025
5 checks passed
phryneas added a commit to apollographql/apollo-client-integrations 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]>
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