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

Bug: React 18 emit null chars in renderToPipeableStream #31134

Closed
slorber opened this issue Oct 7, 2024 · 13 comments
Closed

Bug: React 18 emit null chars in renderToPipeableStream #31134

slorber opened this issue Oct 7, 2024 · 13 comments
Labels
Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug

Comments

@slorber
Copy link
Contributor

slorber commented Oct 7, 2024

React version: 18.3.1

Steps To Reproduce

Replace renderToString(app) by renderToPipeableStream(app).

The Docusaurus SSG framework own website generates static pages with React, and after the change, some of them will start containing NULL chars \0.

The current behavior

  • renderToString(app) does not emit NULL chars \0
  • renderToPipeableStream(app) emit NULL chars \0`

The expected behavior

  • renderToString(app) does not emit NULL chars \0
  • renderToPipeableStream(app) does not emit NULL chars \0`

Details

renderToPipeableStream is not a 1-1 replacement for renderToString, but I'm using the following code to obtain a string:

const {PassThrough} = require('node:stream');
const {text} = require('node:stream/consumers');

async function renderToHtml(app) {
    return new Promise((resolve, reject) => {
        const passThrough = new PassThrough();
        const {pipe} = ReactDOMServer.renderToPipeableStream(app, {
            onError(error) {
                reject(error);
            },
            onAllReady() {
                pipe(passThrough);
                text(passThrough).then(resolve, reject);
            },
        });
    });
}

I also tried various alternatives suggested by @sebmarkbage, @gnoff and the community in this Twitter thread, including:

  • new Response(webStream).text()
  • TextEncoder.decode(chunk,{stream: true})

All the methods I tried to convert renderToPipeableStream did have occasional NULL chars, unlike renderToString, so I assume it is, in the end, a React bug.

See related Docusaurus issue: facebook/docusaurus#9985

Runnable repro

A full repro is provided here: https://github.com/slorber/react-bug-repro-null-chars

It is standalone, but unfortunately not very minimal because it contains the full server bundle we run SSR/SSR with for the Docusaurus website, and also bundles React and ReactDOMServer.

The repro code:

const {PassThrough, Writable} = require('node:stream');
const {text} = require('node:stream/consumers');
const {
    default: renderApp,
    ReactDOMServer,
} = require('./__server/server.bundle');

async function repro() {
    console.log('REPRO START');
    await renderPathname('/blog/releases/3.0');
    await renderPathname('/blog/releases/3.1');
    await renderPathname('/blog/releases/3.2');
    await renderPathname('/blog/releases/3.3');
    await renderPathname('/blog/releases/3.4');
    await renderPathname('/blog/releases/3.5');
    console.log('REPRO END');
}

repro();

async function renderPathname(pathname) {
    const {app} = await renderApp({
        pathname,
    });

    const htmlRef = await renderToHtmlReference(app);
    const htmlStream1 = await renderToHtmlStream1(app);
    const htmlStream2 = await renderToHtmlStream2(app);

    if (htmlStream1 !== htmlRef || htmlStream2 !== htmlRef) {
        console.error(`HTML difference detected for pathname=${pathname}
htmlRef.length=${htmlRef.length} (${countNulls(htmlRef)} nulls)
htmlStream1.length=${htmlStream1.length} (${countNulls(htmlStream1)} nulls)
htmlStream2.length=${htmlStream2.length} (${countNulls(htmlStream2)} nulls)
    `);
    } else {
        console.log(`Successfully rendered the same HTML for pathname=${pathname}`);
    }
}

function countNulls(str) {
    return (str.match(/\0/g) || []).length;
}

async function renderToHtmlReference(app) {
    return ReactDOMServer.renderToString(app);
}

async function renderToHtmlStream1(app) {
    return new Promise((resolve, reject) => {
        const passThrough = new PassThrough();
        const {pipe} = ReactDOMServer.renderToPipeableStream(app, {
            onError(error) {
                reject(error);
            },
            onAllReady() {
                pipe(passThrough);
                text(passThrough).then(resolve, reject);
            },
        });
    });
}

async function renderToHtmlStream2(app) {
    class WritableStream extends Writable {
        html = '';
        decoder = new TextDecoder();
        _write(chunk, enc, next) {
            this.html += this.decoder.decode(chunk, {stream: true});
            next();
        }
        _final() {
            this.html += this.decoder.decode();
        }
    }

    return new Promise((resolve, reject) => {
        const {pipe} = ReactDOMServer.renderToPipeableStream(app, {
            onError(error) {
                reject(error);
            },
            onAllReady() {
                const writeableStream = new WritableStream();
                pipe(writeableStream);
                resolve(writeableStream.html);
            },
        });
    });
}

Similarly to the Docusaurus website, only the path /blog/releases/3.5 contains NULL chars, and the other tested ones do not:

node repro.js

REPRO START
Successfully rendered the same HTML for pathname=/blog/releases/3.0
Successfully rendered the same HTML for pathname=/blog/releases/3.1
Successfully rendered the same HTML for pathname=/blog/releases/3.2
Successfully rendered the same HTML for pathname=/blog/releases/3.3
Successfully rendered the same HTML for pathname=/blog/releases/3.4

HTML difference detected for pathname=/blog/releases/3.5
htmlRef.length=50894 (0 nulls)
htmlStream1.length=50896 (2 nulls)
htmlStream2.length=50896 (2 nulls)

REPRO END

This problematic page is an MDX release blog post, and the NULL char occurs in the middle of it.

For some unknown reasons, modifying the beginning of the post (adding or removing Markdown **) randomly makes the NULL chars disappear, even if it's far from the NULL occurrence position. Note that the compiled output of MDX doesn't contain NULLs. For these reasons, it might be challenging to strip down the repro and create a more minimal one, although I could try.

Follow-up

This is an initial bug report that I plan to complete depending on your answers:

  • Can we expect a bug like this to be fixed in 18.x?
  • Is this already fixed in v19?
  • Do you have enough information to investigate?
  • Should I invest more time into creating a smaller repro?
  • Do you think html.replace(/\0/g, '') is a good/safe temporary workaround?

Thanks


Edit: the following method using renderToReadableStream behaves as expected (exactly like renderToString) and doesn't produce extra NULL chars:

import type {ReactNode} from 'react';
import {renderToReadableStream} from 'react-dom/server.browser';
import {text} from 'stream/consumers';

export async function renderToHtml(app: ReactNode): Promise<string> {
  const stream = await renderToReadableStream(app);
  await stream.allReady;
  return text(stream);
}

So it looks like the problem only affects renderToPipeableStream

We might use this method in Docusaurus to fix our problem: facebook/docusaurus#10562

@sebmarkbage
Copy link
Collaborator

Just to be clear, you're emitting actual NULL characters in the strings in the text content, right?

Sadly, I think this is a bug in renderToString and maybe renderToReadableStream if it filters it out. Because SSR should be have the same as the client. Otherwise it would yield different results and hydration mismatches. Sadly it seems we have code to ignore those hydration mismatches by normalizing the string but it really should be a mismatch.

What we probably should be doing instead is escape NULL characters into HTML entities so that they don't have to be handled by parsers but should still be preserved.

@gnoff
Copy link
Collaborator

gnoff commented Oct 7, 2024

@slorber can you update to React 19 canary in a branch and push that up? we don't minify production builds there. It's posssible the bug will go away with the upgrade but that's not actually comforting because what is almost certainly happening is there is a character on the boundary of two chunks (2048 bytes) that uses more than one utf-8 code point and so small shifts in the content or chunk size can cause this character to no longer land on a boundary. But if it does repro in 19 it will be easier to track down what is going on. Also is the mdx for the 3.5 available in the repro? It seems embedded in the compiled project so I can't quite see all the input strings unless I'm misinterpretting the repro repository

@slorber
Copy link
Contributor Author

slorber commented Oct 7, 2024

Thanks for taking a look

Docusaurus isn't using React 19 yet so I'll have to make a POC of upgrade to see if I can reproduce. I'll investigate more at the end of the week.

The problematic blog post source is https://github.com/facebook/docusaurus/blob/main/website/blog/releases/3.5/index.mdx

I checked and our MDX sources do not contain NULL chars, nor the MDX compiled output. And I tried with both Webpack/Rspack so it's probably not a bundler bug unless they ported it to Rspack.

@tats-u
Copy link

tats-u commented Oct 10, 2024

I saw this issue in Remix too.
This is not a Docusaurus-specific or MDX-related issue.
I don't use MDX or remark in my Remix app where I found this issue.
Chinese and Korean versions of the top page of Docusaurus contain NULL, too, which shows this issue is not related to MDX.

Update: they don't contain NUL today just thank to a monkey patch by Docusaurus itself.

@tats-u
Copy link

tats-u commented Oct 15, 2024

#24985 looks similar to this bug to some extent.

@slorber
Copy link
Contributor Author

slorber commented Oct 15, 2024

Yes it looks very similar to this other bug fixed by @sophiebits in #26228

However in our case it doesn't seem to be fixed under v18.3.0

Similarly, I can confirm the bug isn't there on v18.0.0 and appears at 18.1.0 (release: https://github.com/facebook/react/releases/tag/v18.1.0)

The change that introduced this problem is probably this @gnoff PR: #24291

@slorber
Copy link
Contributor Author

slorber commented Oct 15, 2024

I can also confirm the bug is not there anymore with 19.0.0-rc.0

@tats-u
Copy link

tats-u commented Oct 15, 2024

I can also confirm the bug is not there anymore with 19.0.0-rc.0

I wonder which version and commit in the canary or next channel this bug is fixed in if you are correct.

@tats-u
Copy link

tats-u commented Oct 16, 2024

Oh, #26228 hasn't been backported to v18.x yet! The latest commit in packages/react-server/src/ReactServerStreamConfigNode.js in v18.3.1 is still #24291!

https://github.com/facebook/react/commits/v18.3.1/packages/react-server/src/ReactServerStreamConfigNode.js
https://github.com/facebook/react/commits/main/packages/react-server/src/ReactServerStreamConfigNode.js

All we have to fix this problem is to backport #26228 to v18.x. I believe it can never cause a bad regression and is safe for existing users of v18.x.

I found a monkey patch for v18.3.1:

sed -i -e '11s/\(d<b.length&&(t(a,k\))/\1.subarray(0,l))/' node_modules/react-dom/cjs/react-dom-server.node.production.min.js
sed -i -e '129s/currentView/currentView.subarray(0, writtenBytes)/' node_modules/react-dom/cjs/react-dom-server.node.development.js

https://unpkg.com/browse/[email protected]/cjs/react-dom-server.node.production.min.js
image
Line 11, Character 243

https://unpkg.com/browse/[email protected]/cjs/react-dom-server.node.development.js
image
Line 129

These are equivalent to the patch on packages/react-server/src/ReactServerStreamConfigNode.js in https://github.com/facebook/react/pull/26228/files#diff-efdf85acf9fb91d560a8a49ac4f6480c2a4c59c4c67a8b3c0201083326495d7a with (currentView: any) replaced with currentView.

Once this monkey patch was applied, NULL characters in my Docusaurus build & Remix app did vanish completely as if they had been enchanted.


If you want to apply this patch using your text editor:

production:

Go to the 143rd character in the line 11, and replacet(a,k) with t(a,k.subarray(0,l)). Remember it as "tackle".
t(a,k.subarray(0,l)) appears once before t(a,k) in the same line.

development:

Go to the line 129 and replace currentView with currentView.subarray(0, writtenBytes).

@slorber
Copy link
Contributor Author

slorber commented Oct 17, 2024

I'm not sure the React team is planning any new v18.x release anymore considering v19 is around the corner.

v18.3 was released only to add extra warnings to help us migrate to v19, and did not contain any bugfix/feature: https://github.com/facebook/react/releases/tag/v18.3.0

I guess we can close as duplicate, already fixed in v19.

React 18 users will have to apply the patch locally with patch-package if they really need to.

@slorber slorber closed this as not planned Won't fix, can't repro, duplicate, stale Oct 17, 2024
@tats-u
Copy link

tats-u commented Oct 17, 2024

It's not duplicated but just changed to a backport request to 18.x.
How about changing the title to something like "Backport #26228 to React 18.x" instead of closing it at your own discretion? 19 is just around the corner and has not been released yet. Not everyone can migrate to v19 immediately.

It's unhealthy to lead an outsider of Meta to maintain something like https://github.com/tats-u/react-dom-no-nul and lead React 18.x users to use it.

@slorber
Copy link
Contributor Author

slorber commented Oct 17, 2024

@tats-u I'm not a Meta employee and not part of the React core team. I have no weight on their decision to release another v18.x. Considering v18.3 was released 2 years after v18.2 with 0 backported bug fixes, my intuition tells me there's no v18.4 release planned. Even before v18.3 they communicated on Twitter no v18.3 was planned.

https://react.dev/blog/2024/04/25/react-19-upgrade-guide

CleanShot 2024-10-17 at 22 40 09@2x

https://x.com/sebastienlorber/status/1780224019398066433

image


This was initially a bug report, and the bug has been fixed in v19. For Docusaurus I apply the same strategy and do not necessarily backport fixes to v2 anymore since we are at v3.5 already, even if there are still many v2 sites. It's a tradeoff many of us have to make to move faster and encourage users to upgrade.

If you want to open a backport request, feel free to do so in your own name in a different issue.

There are decent workarounds such as using renderToString or patch-package.

@tats-u
Copy link

tats-u commented Oct 20, 2024

It's disappointing, but we don't have to use extra packages to patch React if we have adopted pnpm or Yarn v2+.
patch-package for npm or Yarn v1 is fine, too.
I'll give up for an official backport.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug
Projects
None yet
Development

No branches or pull requests

4 participants