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

[Flight Parcel] Implement prepareDestinationForModule #31799

Merged
merged 4 commits into from
Dec 31, 2024

Conversation

devongovett
Copy link
Contributor

Followup to #31725

This implements prepareDestinationForModule in the Parcel Flight client. On the Parcel side, the <Resources> component now only inserts <link> elements for stylesheets (along with a bootstrap script when needed), and React is responsible for inserting scripts. This ensures that components that are conditionally dynamic imported during render are also preloaded.

CSS must be added to the RSC tree using <Resources> to avoid FOUC. This must be manually rendered in both the top-level page, and in any component that is dynamic imported. It would be nice if there was a way for React to automatically insert CSS as well, but unfortunately prepareDestinationForModule only knows about client components and not CSS for server components. Perhaps there could be a way we could annotate components at code splitting boundaries with the resources they need? More thoughts in this thread: #31725 (comment)

Copy link

vercel bot commented Dec 16, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-compiler-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 16, 2024 4:49am

function Content() {
data ??= createFromReadableStream<ReactElement>(s1);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW this took me a long time to debug why preinits weren't working. This must be initialized during render so the current request is set in AsyncLocalStorage. Might be nice if there was some kind of error/warning instead of silently not inserting any scripts.

Copy link
Collaborator

@sebmarkbage sebmarkbage Dec 31, 2024

Choose a reason for hiding this comment

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

An issue is that it's legit to have an optimistic preload or preinit in module scope. E.g. the first time you load a module on the client. The silent ignoring allows those same modules to keep working with SSR or RSC albeit without the preload.

However, this is also related to another issue that it should be possible to consume values from the RSC stream (like formState) before SSR:ing the payload. It's also useful for other routing concerns. You can see this awkwardness in the fixtures/flight where it has to parse it twice to deal with the cycle of somethings having to be outside the render and somethings inside.

This is also related to console replaying when doing RSC to RSC since the replays have to be in the scope that needs to replay.

Another case is expanding the React.cache scope to longer than just one RSC render.

We have plans to add something that allows expanding the scopes for these AsyncLocalStorage contexts. It's just a little unclear the exact shape atm.

@react-sizebot
Copy link

Comparing: e06c72f...82864ad

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.js = 6.68 kB 6.68 kB +0.11% 1.83 kB 1.83 kB
oss-stable/react-dom/cjs/react-dom-client.production.js = 510.76 kB 510.76 kB = 91.36 kB 91.36 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.69 kB 6.69 kB +0.05% 1.83 kB 1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js = 515.55 kB 515.55 kB = 92.17 kB 92.17 kB
facebook-www/ReactDOM-prod.classic.js = 594.69 kB 594.69 kB = 104.93 kB 104.93 kB
facebook-www/ReactDOM-prod.modern.js = 584.96 kB 584.96 kB = 103.37 kB 103.37 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable-semver/react-server-dom-parcel/cjs/react-server-dom-parcel-client.node.production.js +1.08% 52.75 kB 53.32 kB +1.61% 10.78 kB 10.96 kB
oss-stable/react-server-dom-parcel/cjs/react-server-dom-parcel-client.node.production.js +1.08% 52.75 kB 53.32 kB +1.61% 10.78 kB 10.96 kB
oss-experimental/react-server-dom-parcel/cjs/react-server-dom-parcel-client.node.production.js +1.07% 53.26 kB 53.83 kB +1.61% 10.86 kB 11.03 kB
oss-stable-semver/react-server-dom-parcel/cjs/react-server-dom-parcel-client.edge.production.js +1.05% 54.28 kB 54.85 kB +1.59% 11.09 kB 11.27 kB
oss-stable/react-server-dom-parcel/cjs/react-server-dom-parcel-client.edge.production.js +1.05% 54.28 kB 54.85 kB +1.59% 11.09 kB 11.27 kB
oss-experimental/react-server-dom-parcel/cjs/react-server-dom-parcel-client.edge.production.js +1.04% 54.79 kB 55.36 kB +1.58% 11.17 kB 11.35 kB
oss-stable-semver/react-server-dom-parcel/cjs/react-server-dom-parcel-client.node.development.js +0.64% 90.04 kB 90.61 kB +1.02% 17.02 kB 17.19 kB
oss-stable/react-server-dom-parcel/cjs/react-server-dom-parcel-client.node.development.js +0.64% 90.04 kB 90.61 kB +1.02% 17.02 kB 17.19 kB
oss-stable-semver/react-server-dom-parcel/cjs/react-server-dom-parcel-client.edge.development.js +0.63% 91.84 kB 92.41 kB +0.98% 17.37 kB 17.54 kB
oss-stable/react-server-dom-parcel/cjs/react-server-dom-parcel-client.edge.development.js +0.63% 91.84 kB 92.41 kB +0.98% 17.37 kB 17.54 kB
oss-experimental/react-server-dom-parcel/cjs/react-server-dom-parcel-client.node.development.js +0.57% 101.76 kB 102.34 kB +0.88% 19.36 kB 19.53 kB
oss-experimental/react-server-dom-parcel/cjs/react-server-dom-parcel-client.edge.development.js +0.56% 103.57 kB 104.14 kB +0.86% 19.69 kB 19.86 kB

Generated by 🚫 dangerJS against 158e5cb

@devongovett devongovett changed the title Implement prepareDestinationForModule in Parcel Flight client [Flight Parcel] Implement prepareDestinationForModule in client Dec 17, 2024
@devongovett devongovett changed the title [Flight Parcel] Implement prepareDestinationForModule in client [Flight Parcel] Implement prepareDestinationForModule Dec 17, 2024
@devongovett
Copy link
Contributor Author

@sebmarkbage if you have a chance, lmk if this approach looks ok and if you have any thoughts on how we could make CSS a bit more automatic.

@sebmarkbage sebmarkbage merged commit 694d3e1 into facebook:main Dec 31, 2024
187 checks passed
@sebmarkbage
Copy link
Collaborator

I think the essential part of making CSS automatic is to somehow detect that the boundary itself is a React Node and converting it into to something that renders basically the equivalent of <Resources> automatically.

For example, detecting that a client reference is only used within a specific JSX call:

import {ClientComponent} from ...
...
<ClientComponent />

Compiles to

import {ClientComponent, Resources} from ...
...
<>
  <Resources />
  <ClientComponent />
</>

Now the inner CSS can be excluded from the outer parent (e.g. route/page).

The same principle can be applied to React specific async loading like React.lazy from Client Components. Since this problem isn't unique to RSC.

const LazyComponent = React.lazy(() => import(...));
...
<LazyComponent />

Compiles to

const LazyComponent = React.lazy(async () => {
  const {default: Component, Resources} = await import(...)
  return {
    default: function Wrapper(props) {
      return <>
        <Resources />
        <Component {...props} />
      </>;
    }
  }
});
...
<LazyComponent />

Just as examples. The key here is that if we just assigned something to the ClientReference or something it might be easy to lose the additional information in a way that's not safe using a wrapper. So it's more about being conservative and only do this when it's safe.

There are ofc, non-React centric ways of lazy loading code. Even RSC lets you pass non-Components.

import {ClientComponent} from ...
import {clientFunction} from ...
...
<ClientComponent createDOMNode={clientFunction} />
"use client";
export function ClientComponent({createDOMNode}) {
  const ref = useRef();
  useEffect(() => {
    ref.current.appendChild(createDOMNode())
  }, [])
  return <div ref={ref} />
}

or the equivalent to React.lazy in just JS:

export function ClientComponent(props) {
  const ref = useRef();
  useEffect(() => {
    import(...).then(({ createDOMNode }) => {
      ref.current.appendChild(createDOMNode())
    });
  }, [])
  return <div ref={ref} />
}

In these cases, the lazy import might itself depend on CSS which needs to be loaded. There's basically two approaches to this. You can either hoist into the parent route so that it's eagerly loaded as part of the parent module loading, or you can do it using the old technique of just treating it like a JS module so it's loaded before the import() resolves.

The downsize of the latter is that it doesn't work if that async dependency ends up as SSR because in this case React wouldn't know that you really needed the CSS for this to load because showing this on the client since there's no suspensey CSS:

const lazy = import(...); // hoisting this out just to show that AsyncLocalStorage isn't enough
export function ClientComponent(props) {
  const getText = use(lazy.then(({default}) => default));
  return getText();
}

The same case with RSC + SSR:

"use client";
export function ClientComponent({ clientFunction }) {
  return clientFunction(); // This might be another Client Reference which depends on CSS.
}

Another tricky case is if the same CSS chunks are depended upon by both a React Node aware module (which can treat it as suspensey) and another plain JS dynamic import() but that's solvable once the other things are solved.

So one possible approach would be:

a) Detect things that are definitely only used by React Nodes and treat those as suspensey CSS.
b) Detect things that are definitely only used outside SSR and treat their CSS as "modules".
c) Hoist anything else to some parent for eager loading (which could just be the root/runtime CSS if there's no other parent).

Another option is to just ignore the SSR problem. I.e. treat all of c as b. That's really the old status quo of the React ecosystem anyway. You didn't used to be able to await arbitrary imports in React before neither. It's a lost standing preexisting problem.

@sebmarkbage
Copy link
Collaborator

sebmarkbage commented Dec 31, 2024

In general we opted to for more hoisting into the parent Page for Client References in Next.js which has the downside that you might overload CSS. However, the overhead added of tracking this at each component when you have a lot of small Client References is also a downside. Adding CSS late can also be a significant performance downside since each time it has to recompute all the styles for all existing DOM nodes essentially so streaming in CSS isn't that beneficial. Combined with the benefits of just using Atomic CSS in general that flattens out over time meant that hoisting into coarser boundaries in general is more beneficial. However, ideally even that should be at the prerogative of the bundler to make the choice since it has more information about things like how large the chunks are.

@sebmarkbage
Copy link
Collaborator

This general problem space is also just a specific version of the problem where native browser CSS modules return a StyleSheet that needs to be injected somewhere instead of a global side-effect.

@devongovett
Copy link
Contributor Author

Yeah, I was thinking about generating wrapper components to inject the Resources automatically like you described. Maybe dynamic importing a module that has a CSS dependency but not rendering anything from it as a JSX element is enough of an edge case that it could be handled manually.

The hard part is actually detecting statically if a dynamic import is used as a JSX element. There are lots of different ways that could happen - property access (could be dynamic or through some function), destructuring, React.lazy, etc.

Another problem is that adding wrapper components messes with the object identity of the component being imported. If someone attached additional properties to their function or something, those would also need to be proxied.

function Component() {
  // ...
}

// This would need to be proxied by the wrapper.
Components.something = 'whatever';

Another idea I thought of is to "tag" the exports of a boundary module with the resources it uses, and then React itself could potentially inject them at runtime. Not sure if this is a good idea. Basically like this:

function Component() {
  // ...
}

// Injected by compiler
Component[Symbol.for('react.resources')] = <>
  <link rel="stylesheet" ... />
</>;

And then when React sees a JSX element with a type that has a resources property attached to it, it could add those resources into the tree as siblings.

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

Successfully merging this pull request may close these issues.

4 participants