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.

Sign up for GitHub

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

Jump to bottom

RFC: injectToStream #219

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

RFC: injectToStream #219

wants to merge 1 commit into from

Conversation

brillout
Copy link

@sebmarkbage
Copy link
Collaborator

sebmarkbage commented May 2, 2022 •
edited
Loading

I agree about the problem statement and urgency. I considered this and intentionally excluded it from the MVP but doesn't mean it can't be added.

The main thing that makes a hook like this less powerful is that I don't think features using it will be future compatible with new features that we have in mind. There are way more things we can do with SSR that we haven't discussed yet. We've also explicitly decided not to add something like this for Server Components.

Therefore our focus has been on adding higher level replacements for each use case.

I expect any solution using this to be temporary in the interim before we add new features that wouldn't work well with them anyway and until we add the higher level alternatives.

That's not to say that adding this in the interim wouldn't be valuable. That's why we have things like useEffect in the first place. Tbh, not adding this is more of a prioritization of work - which effectively shifts the burden to others to deal with the intermediate step. This is unfortunate but the reality.

Another proposal is to just have every SSR library add an API like this. Technically you don't need the React team to add this. It would require agreement and coordination but so would adding it to React. Each of them would just have to commit to an upgrade path. It just side-steps the React team as the bottleneck and maintainers.

To get a sense for other RFCs coming that you can expect will touch of this problem space:

  • Inserting deeply nested <head> (not intended for data, styling or script injection).
  • Managing preloading scripts and external CSS files.
  • Inserting preloads/early loads for React.lazy scripts discovered during SSR.
  • Inserting preloads of images discovered during SSR.
  • Inserting <link rel="stylesheet" /> tags in a Suspensey way both on the server and client.
  • Inserting data that was fetched during SSR into the HTML for hydration.
  • Special treatment of Server Components streams when they're the data.

These would also work from Server Components.

A notable missing one is CSS-in-JS that use dynamic <style> tag generation. I see a lot of issues with how that plays with some future features (the first one being Server Components). We haven't figured out an API that I think works well. We already added useInsertionEffect on the client but we don't have an equivalent on the server. We'd have to add something though even if it has downsides when used with other features. If it doesn't work well with other features anyway, then maybe it's fine that it's a low level API such as this one.

None of this is to say that we can't/shouldn't add this but this is just why we haven't done it yet. If we do, it's urgent since it's more as an interim feature.

@brillout
Copy link
Author

brillout commented May 3, 2022

Thanks for the insightful answer, I appreciate it.

Ok, I see the reluctance because of potential future breaking changes, but the way I see it is that this RFC (or any alternative that enables library authors to inject to the stream) doesn't make the situation worse: React's 18 discussion board already recommends injecting tags to the stream such as style tags, data <script type="application/json"> and JS <script src="script.js"> tags. This means that library authors are already injecting to the stream, and this RFC doesn't introduce any potential future breaking changes that don't already exist today.

Another proposal is to just have every SSR library add an API like this. Technically you don't need the React team to add this. It would require agreement and coordination but so would adding it to React. Each of them would just have to commit to an upgrade path. It just side-steps the React team as the bottleneck and maintainers.

The thing I'm worried about is the feasibility to align all SSR frameworks.

If all SSR frameworks (Next.js, Hydrogen, ...) were to use react-streaming (or an alternative) then that we solved the problem: library authors (Telefunc, GraphQL, ...) would then use react-streaming's implementation of injectToStream(). But I can't really convince all SSR frameworks to use react-streaming.

Instead of all SSR framework agreeing on using react-streaming, the more likely outcome is that each SSR frameworks ends up implementing their own injectToStream(). This means that library authors will have to use n injectToStream() variants where n is the number of frameworks.

We can expect SSR frameworks to spend time to implement their own injectToStream(), but I don't think we should expect library authors, which are much more resource limited, to need to use n different injectToStream() interfaces to be able to inject to the stream.

Tbh, not adding this is more of a prioritization of work - which effectively shifts the burden to others to deal with the intermediate step. This is unfortunate but the reality.

I see. So I guess it boils down to how we can design the interm solution so that:

  1. It enables tools like react-streaming to seamlessly work accross all SSR frameworks.
  2. It's quick for React to implement.

Library authors (Telefunc, GraphQL, ...) can then use higher-level abstraction such as react-streaming to easily inject what they need to the stream, without worrying about framework integration.

This RFC is a solution for 1. but I can't say if there is a better solution for 2.

@sebmarkbage
Copy link
Collaborator

@brillout If we didn't do this and picked one higher level use case to prioritize. If you had to pick one use case, which one would you pick?

@brillout
Copy link
Author

This one:

  • Inserting data that was fetched during SSR into the HTML for hydration.

By far. Simply because it's required to have suspended SSR boundaries.

Without passing SSR data to the client-side, we cannot achieve suspended SSR boundaries for data fetching. Which defeats the purpose of SSR streams.

These other use cases seem like low-priority nice-to-have optimizations:

  • Inserting deeply nested <head> (not intended for data, styling or script injection).
  • Managing preloading scripts and external CSS files.
  • Inserting preloads/early loads for React.lazy scripts discovered during SSR.
  • Inserting preloads of images discovered during SSR.
  • Inserting <link rel="stylesheet" /> tags in a Suspensey way both on the server and client.

As for:

  • Special treatment of Server Components streams when they're the data.

I'm not all too familiar with Server Components, so I can't say. But since Server Components are not widely adopted yet, I'm thinking it's probably low priority as well.


Also, I'm thinking maybe something like react-streaming's useAsync() and useSsrData() could be high-level contracts between React and the users / library authors.

It seems to me that having useAsync() built into React to be quite useful for users and, AFAICT, useAsync() is a future-safe feature.

Same for useSsrData() but for library authors. (E.g. React Query would want to control the cache key.)

Let me know if you have questions, I'm happy to elaborate.

@shuding shuding mentioned this pull request Jul 4, 2022
Open
@gaearon
Copy link
Member

gaearon commented Jul 20, 2022

Some initial work on the stylesheet case is being done in facebook/react#24886

@mashaal mashaal mentioned this pull request Nov 2, 2022
Closed
@brillout
Copy link
Author

work on the stylesheet case

👍 Also about the data fetching use case: https://github.com/acdlite/rfcs/blob/first-class-promises/text/0000-first-class-support-for-promises.md. I didn't read the whole RFC yet, but it seems like it completely replaces the need for react-streaming. That's neat.

@brillout brillout mentioned this pull request Mar 2, 2023
Closed
@phryneas
Copy link

This is something we would really need in Apollo-Client to add good support for the new renderToReadableStream/renderToPipeableStream SSR (not RSC, that's a different set of problems!).

Right now, we are considering doing the following:

  • Whenever request data is received on the server, we queue it using NextJS's ServerInsertedHTMLContext (an implementation detail of useServerInsertedHTML), which is meant to insert css during SSR.
  • We replay that request data on the browser for cache entries to be added to Apollo Client

(Additionally, we also "snapshot" the return value of specific hooks to rehydrate them on the client side with the right value even if the client cache has already advanced to the future, but that's not relevant to this issue.)

Our current approach has a few issues.

  • It's next.js specific, and we would like to support other SSR frameworks without creating an extra package for every framework (and possibly spending weeks on finding out how to support them best).
  • If flushes data to the client every time a new suspense boundary is done. That can often be too late and result in race conditions - the client might have made a similar request after the server made it, and replaying the response the server forwarded to the browser with a long delay might set a the whole app "back in time". The ability to stream data "as soon as possible" and not "when the next suspense boundary is done" would be incredibly helpful here to minimize this risk.
  • If we were to try and build a solution ourselves that used something like AsyncLocalStorage/AsyncContext to pass down the stream to inject into, we would run into a chicken-and-egg problem for calling createReadableStream, and even with createPipeableStream we would have to add a lot of logic to detect when React is currently streaming data, as not to inject data in-between React chunks. With time passing on, this would stay a "moving target" as React keeps adding more features - React has more access to the internal state, so that would seem to be the right place to implement this.

Is there any way for this to be reconsidered/reprioritized?
If it's just about implementation work: I would be happy to help with that, but I would need some guidance on how to implement this best.

@phryneas phryneas mentioned this pull request May 26, 2023
Open
@phryneas phryneas mentioned this pull request Jun 7, 2023
Closed
@dac09
Copy link

dac09 commented Jul 27, 2023

Great write out and RFC!

It does seem like an important omission from the React's streaming capabilities at the moment, and it seems to me (with my limited knowledge) like an antipattern for data-fetching library authors to have to fork special versions for each framework.

The workarounds sadly aren't documented anywhere that I can see! Is there a safe way to inject to the stream with an intermediate stream? It doesn't seem that way to me right now as there's no guarantee that each of Reacts chunks are "complete" .

Right now in Redwood, we will have to use the "ServerHtmlContext" workaround used in react-streaming and NextJS, but an indication from the React team of the direction we would be going in (and some idea if this is being prioritised) would be great. :)

FWIW - we are looking at this without using RSC capabilities stand point at first - just streaming+ssr, before we move into the RSC world.

@brillout brillout mentioned this pull request May 24, 2024
Open
This was referenced Jul 3, 2024
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@brillout @sebmarkbage @gaearon @phryneas @dac09 @facebook-github-bot