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

[React 19] Cannot assign to readonly property #30172

Open
hipstersmoothie opened this issue Jul 1, 2024 · 7 comments · May be fixed by hipstersmoothie/waku-repro#1
Open

[React 19] Cannot assign to readonly property #30172

hipstersmoothie opened this issue Jul 1, 2024 · 7 comments · May be fixed by hipstersmoothie/waku-repro#1
Labels

Comments

@hipstersmoothie
Copy link

Summary

I'm using https://waku.gg to use react components. I was trying to upgrade to the latest 19 rc and I started getting getting this error:

image

I boiled down the app into just a few file to demonstrate the issue here

The issue is as follow:

// Client context Provider
import { StoryContextProvider } from "../../components/Context";
// Server Component
import A11yDecorator from "../../A11yDecorator";
// Client Component
import CenteredDecorator from "../../CenteredDecorator";

export default function Iframe() {
  const page = {};

  return (
    <StoryContextProvider value={{ page }}>
      <CenteredDecorator page={page}>
        <A11yDecorator page={page}>
          foo
        </A11yDecorator>
      </CenteredDecorator>
    </StoryContextProvider>
  );
}

Some observations:

  • If I switch the order of A11yDecorator and CenteredDecorator, the app renders
  • If I don't use a shared page variable and just pass objects to the relevant places, the app renders
  • If I spread the page and make an new object when passing to the relevant places, the app renders
@hipstersmoothie hipstersmoothie changed the title [React 19] Cannot assing to readonly property Jul 1, 2024
@eps1lon
Copy link
Collaborator

eps1lon commented Jul 1, 2024

You should file this against Waku first to check if this is a framework or React issue. Waku may not be compatible with React 19 yet.

@hipstersmoothie
Copy link
Author

I did. The example code is pretty light. I could potentially test against next.js too

cc @dai-shi

@dai-shi
Copy link
Contributor

dai-shi commented Jul 2, 2024

I originally thought it's a Waku's issue. But, as it turns out, it reproduces with changing React beta, without changing Waku.

@hipstersmoothie I would recommend to find which commit in React causes the error, and look for the background of the change. My gut feeling is that this change is intentional (for safety or something).

@eps1lon
Copy link
Collaborator

eps1lon commented Jul 2, 2024

// Server Component
import A11yDecorator from "../../A11yDecorator";

This is a client component according to the repro: https://github.com/hipstersmoothie/waku-repro/blob/aec12ed7c9594950987157242f80cec86957a71e/src/A11yDecorator.tsx#L1

You probably mean CenteredDecorator which is a Server Component?

@eps1lon
Copy link
Collaborator

eps1lon commented Jul 2, 2024

I can't repro this in React's test suite (#30194) but I can repro it in Next.js (https://github.com/eps1lon/waku-repro-as-nextjs):

TypeError: Cannot assign to read only property 'page' of object '#<Object>'
    at eval (webpack-internal:///(app-pages-browser)/./node_modules/next/dist/compiled/react-server-dom-webpack/cjs/react-server-dom-webpack-client.browser.development.js:2081:23)
    at wakeChunk (webpack-internal:///(app-pages-browser)/./node_modules/next/dist/compiled/react-server-dom-webpack/cjs/react-server-dom-webpack-client.browser.development.js:1739:5)
    at initializeModelChunk (webpack-internal:///(app-pages-browser)/./node_modules/next/dist/compiled/react-server-dom-webpack/cjs/react-server-dom-webpack-client.browser.development.js:1933:9)
    at getOutlinedModel (webpack-internal:///(app-pages-browser)/./node_modules/next/dist/compiled/react-server-dom-webpack/cjs/react-server-dom-webpack-client.browser.development.js:2148:7)
    at parseModelString (webpack-internal:///(app-pages-browser)/./node_modules/next/dist/compiled/react-server-dom-webpack/cjs/react-server-dom-webpack-client.browser.development.js:2404:18)
    at Array.eval (webpack-internal:///(app-pages-browser)/./node_modules/next/dist/compiled/react-server-dom-webpack/cjs/react-server-dom-webpack-client.browser.development.js:3260:14)
    at JSON.parse (<anonymous>)
    at parseModel (webpack-internal:///(app-pages-browser)/./node_modules/next/dist/compiled/react-server-dom-webpack/cjs/react-server-dom-webpack-client.browser.development.js:3252:15)
    at initializeModelChunk (webpack-internal:///(app-pages-browser)/./node_modules/next/dist/compiled/react-server-dom-webpack/cjs/react-server-dom-webpack-client.browser.development.js:1918:17)
    at Chunk.then (webpack-internal:///(app-pages-browser)/./node_modules/next/dist/compiled/react-server-dom-webpack/cjs/react-server-dom-webpack-client.browser.development.js:1643:7)
@unstubbable
Copy link
Contributor

This would be a test in ReactFlightDOMBrowser-test.js that reproduces the issue:

it('should resolve deduped objects in nested children of blocked models', async () => {
  let resolveOuterClientComponentChunk;
  let resolveInnerClientComponentChunk;

  const ClientOuter = clientExports(
    function ClientOuter({children, value}) {
      return children;
    },
    '1',
    '/outer.js',
    new Promise(resolve => (resolveOuterClientComponentChunk = resolve)),
  );

  function PassthroughServerComponent({children}) {
    return children;
  }

  const ClientInner = clientExports(
    function ClientInner({children}) {
      return JSON.stringify(children);
    },
    '2',
    '/inner.js',
    new Promise(resolve => (resolveInnerClientComponentChunk = resolve)),
  );

  const value = {};

  function Server() {
    return (
      <ClientOuter value={value}>
        <PassthroughServerComponent>
          <ClientInner>{value}</ClientInner>
        </PassthroughServerComponent>
      </ClientOuter>
    );
  }

  const stream = await serverAct(() =>
    ReactServerDOMServer.renderToReadableStream(<Server />, webpackMap),
  );

  function ClientRoot({response}) {
    return use(response);
  }

  const response = ReactServerDOMClient.createFromReadableStream(stream);
  const container = document.createElement('div');
  const root = ReactDOMClient.createRoot(container);

  await act(() => {
    root.render(<ClientRoot response={response} />);
  });

  expect(container.innerHTML).toBe('');

  await act(() => {
    resolveOuterClientComponentChunk();
  });

  await act(() => {
    resolveInnerClientComponentChunk();
  });

  expect(container.innerHTML).toBe('{}');
});

But it's basically the same scenario as the test should resolve deduped objects that are themselves blocked, which was added in #29823, and this PR also fixed the bug.

@unstubbable unstubbable linked a pull request Jul 2, 2024 that will close this issue
@dai-shi
Copy link
Contributor

dai-shi commented Jul 2, 2024

So, is it an already fixed bug? I wonder if -rc.1 will be released.

unstubbable added a commit to unstubbable/react that referenced this issue Jul 3, 2024
The issue reported in facebook#30172 was fixed with facebook#29823. This PR also added
the test [`should resolve deduped objects that are themselves
blocked`](https://github.com/facebook/react/blob/6d2a97a7113dfac2ad45067001b7e49a98718324/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js#L348-L393),
which tests a similar scenario. However, this test would have also
succeeded before applying the changes from facebook#29823. Therefore, I believe
it makes sense to add this additional test, which does not succeed
without facebook#29823, to prevent regressions.
sebmarkbage pushed a commit that referenced this issue Jul 3, 2024
The issue reported in #30172 was fixed with #29823. The PR also added
the test [`should resolve deduped objects that are themselves
blocked`](https://github.com/facebook/react/blob/6d2a97a7113dfac2ad45067001b7e49a98718324/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js#L348-L393),
which tests a similar scenario. However, the existing test would have
also succeeded before applying the changes from #29823. Therefore, I
believe it makes sense to add an additional test `should resolve deduped
objects in nested children of blocked models`, which does not succeed
without #29823, to prevent regressions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 participants