-
-
Notifications
You must be signed in to change notification settings - Fork 10.6k
feat: turbo-stream v3 #12945
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
feat: turbo-stream v3 #12945
Conversation
🦋 Changeset detectedLatest commit: 66d5af8 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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.
Did a quick code review - can pull it down and do some app testing too later today or tomorrow if you'd like me too also
@@ -0,0 +1,6 @@ | |||
--- | |||
"integration": minor |
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 dont think we need entries for this
"integration": minor |
@@ -230,6 +230,10 @@ export const createRequestHandler: CreateRequestHandlerFunction = ( | |||
); | |||
} | |||
|
|||
if (!response) { | |||
return new Response("Unknown Server Error", { status: 500 }); |
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.
We can probably run this through the same utility we use elsewhere
return new Response("Unknown Server Error", { status: 500 }); | |
let error = new Error('Unhandled request') | |
return returnLastResortErrorResponse(error, serverMode); |
@@ -327,44 +327,22 @@ export function encodeViaTurboStream( | |||
|
|||
return encode(data, { | |||
signal: controller.signal, | |||
redactErrors: | |||
serverMode === ServerMode.Development ? false : "Unexpected Server Error", |
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.
Nice! This is a great new feature for turbo-stream
👍
Is this backwards compatible with turbo-stream v2? i.e. if the server has updated to v3 but the client hasn't reloaded the window and is still on v2 and makes a data request, what will happen? |
It is not a BW compat data format. |
any update on landing this? |
any updates when this will be merged? |
No updates at the moment - we're still noodling on this internally. This would be a breaking change if we adopted it and with RSC on the horizon it may not be worth it since we'll likely be replacing turbo-stream with the RSC wire format with that work. |
I prefer the turbo-stream format compared to RSC, turbo-stream supports Error instances, async iterables, buffers and a lot more that RSC does not. I think the best approach would be to embed the RSC stream inside turbostream, as a text stream (which is already supported). This way react-router can continue to extend the |
Someone should audit the turbo-stream re-write... It is, after-all, a weekend project that only a handful of people have glanced at. |
It seems that RSC supports async generators now which was one of the main reasons I preferred turbostream |
@brophdawg11 @jacob-ebey Wasn't the whole idea of using |
@MichaelDeBoey I guess the breaking change is a problem for the browser, a client of one version no longer works with a server with another version |
@remorses I don't see how that would be a problem tbh. Once a new deploy of the app is happening, this is going to update both client and server, no? |
It's a problem when client and server are deployed separately, or when an user tries to submit a form when the page was using a previous react-router version compared to the new server deployment |
@remorses The server is the one providing all the client files, so it's not really a use-case that both are deployed separately I think? |
@MichaelDeBoey The server and client files on the server are deployed simultaneously. But clients that had already loaded the page before the server update would now have an incompatible client. They would need a full page reload to download the current version of client-side js. So if the user was about to have some dynamic content show up or submit a form and the server was updated with an incompatible version of turbo-stream, then the interaction could potentially break. On my application, where the clients don't have to do full page reloads very often, the client version can get old. I do intend on showing a notification that there is a new version, so please refresh, but the client would be broken in even showing that message if the endpoint used to convey that information used turbo-stream and the client suddenly couldn't decode the response. A scheduled time to switch over would be ideal in that case. |
@ngbrown I understand your explanation, but couldn't that be the case for a lot more implementation details? Say we change how something on the server is handled, in your explanation that would always be a breaking change. Not sure how the team is looking at this, but unless there are other concerns that I'm currently missing (which could probably be the case), this PR is just a change in implementation details, which shouldn't cause a breaking change in my opinion. |
@MichaelDeBoey But there were a lot of major releases like that, but they were all enabled one-by-one with future flags. So in the end, the breaking change could be rolled out in a controlled way, and the major version of the framework didn't have to increment each time. |
RSC is coming. |
No description provided.