facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
229.04k stars 46.86k forks source link

[React 19] Inconsistent "cache" api with Async Operations in react-server #29955

Open nestarz opened 4 months ago

nestarz commented 4 months ago

Description:

When running the provided code with node, I encounter an issue where the cache is inconsistent depending on whether an async operation is present.

Steps to Reproduce:

  1. Run the following code with node:
const { renderToReadableStream } = require("react-server-dom-webpack/server.edge");
const { cache, createElement } = require("react");

const getId = cache(() => ({ id: Math.random() }));

const A = async () => {
    await new Promise(setImmediate);
    const id = getId();
    console.log(id);
    return null;
};

const B = async () => {
    const id = getId();
    console.log(id);
    return createElement(A, {}, null);
};

renderToReadableStream(createElement(B, {}, null), null);
  1. Observe the console output. The id values logged in A and B are different.
  2. Modify the code to remove the await new Promise(setImmediate); line from A:
const A = async () => {
  const id = getId();
  console.log(id);
  return null;
};
  1. Run the modified code with node again.
  2. Observe the console output. The id values logged in A and B are now the same.

Expected Behavior:

The id values should be consistent regardless of the presence of async operations during the render.

Actual Behavior:

The id values are different when an async operation is present in the render method.

Reproduction Code:

You can run the code using the following command:

npm run start

or a codesandbox: https://codesandbox.io/p/github/nestarz/react-19-cache-issue/main?import=true and the github repo: https://github.com/nestarz/react-19-cache-issue

Additional Information:

It seems there might be an issue with the request context propagation when async operations are involved during the render process. This leads to inconsistent behavior which could cause unpredictable bugs.

Environment:

It impacts the consistency and reliability of cache in async rendering scenarios, and it's critical when used as an alternative to server context, is this normal ? Thank you!

sebmarkbage commented 4 months ago

Note that the .edge package is not intended to be used in Node.js. At least not without more fully controlling the environment like Next.js is doing but even then it's not even recommended for Next.js.

You should be using react-server-dom-webpack/server (which in the "node" condition resolves to react-server-dom-webpack/server.node).

There are many subtle things that are environment dependent. For example this check is a legacy check that looks for AsyncLocalStorage on the global object because importing it doesn't work in some "edge" environments or requires a flag.

https://github.com/facebook/react/blob/main/packages/react-server/src/forks/ReactFlightServerConfig.dom-edge.js#L17

So you'd need to set it up to expose it on the global but that's really something we're intending to change once all environments catch up to have it.

But there are other things that lead to much worse performance when you use Edge in Node.js due to the streaming as well as the edge using setTimeout for scheduling. There's also debugging information that will only be available using the Node.js build.

Therefore, using the "edge" build in Node.js is currently not really supported.

nestarz commented 4 months ago

Thanks @sebmarkbage, in which environment is the edge build meant to work?

I figured out how to create a simple reproduction here, but my real issue arises when I use "react-server-dom-esm/server.node," which I have built from the source.

Here is a CodeSandbox: https://codesandbox.io/p/github/nestarz/react-19-cache-issue/jsr And the related built file: https://jsr.io/@bureaudouble/rsc-engine/0.0.101/vendor/react-server-dom-esm/cjs/react-server-dom-esm-server.node.development.js

So, here it's not because of the runtime but because of the "esm" build. I know it's not well-supported either, but can you point me to a fix so that AsyncLocalStorage works better?

For disclosure, I'm trying to build a simple meta-framework for use in multiple runtimes, like Node and Deno. Instead of using webpack, I solely rely on esbuild for client components and their dependencies, and make use of import-maps for resolution. The framework works well; see here: https://github.com/nestarz/bureaudouble-rsc-demo/ The only missing piece is a working request context preservation throughout the render lifecycle and through multiple async operations; and I'm kind of confident that the issue is from the react codebase.

github-actions[bot] commented 1 month ago

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

nestarz commented 1 month ago

I'm still affected by this issue, the AsyncStorage usage by the cache api is not supported even on the esm/node version and I provided a codesandbox with a minimal reproduction code to help.