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

Deprecate renderToNodeStream (and fix textarea bug) #23359

Merged
merged 4 commits into from
Feb 25, 2022

Conversation

sebmarkbage
Copy link
Collaborator

@sebmarkbage sebmarkbage commented Feb 24, 2022

This adds a deprecation warning for renderToNodeStream and favors the new renderToPipeableStream.

Notably renderToStaticNodeStream is NOT deprecated yet. That's because the static optimizations are only in the legacy build right now. I don't really want to add that flag to the main line so we should do something special for the e-mail generation use case with a special build. So we don't have an alternative yet. That also means there's no static option for Web Streams.

I switched our integration tests to use renderToPipeableStream. That revealed some breaking tests. All due to the fact that we add trailing comment nodes to any text node. This is pretty clowny and known optimization that we know we need to add back in. The only reason it didn't break before is because the legacy version strips it out at the end.

However, in the case of "textarea" that's actually a bug since that will show up in the textarea's value. So I fixed that bug.

This is the equivalent API. This means that we have way less test coverage
of this API but I feel like that's fine since it has a deprecation warning
in it and we have coverage on renderToString that is mostly the same.
The test changes revealed a bug with textarea. It happens because we
currently always insert trailing comment nodes. We should optimize that
away. However, we also don't really support complex children so we
should toString it anyway which is what partial renderer used to do.
These tests are unnecessarily specific about number of nodes.

I special case these, which these tests already do, because they're good
tests to test that the optimization actually works later when we do
fix it.
@sizebot
Copy link

sizebot commented Feb 24, 2022

Comparing: 8c4cd65...509678d

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.min.js = 131.26 kB 131.26 kB = 41.99 kB 41.99 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 136.12 kB 136.12 kB = 43.42 kB 43.42 kB
facebook-www/ReactDOM-prod.classic.js = 434.31 kB 434.31 kB = 79.42 kB 79.42 kB
facebook-www/ReactDOM-prod.modern.js = 424.09 kB 424.09 kB = 77.98 kB 77.98 kB
facebook-www/ReactDOMForked-prod.classic.js = 434.31 kB 434.31 kB = 79.42 kB 79.42 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 509678d

@sebmarkbage sebmarkbage merged commit 8d0d0e9 into facebook:main Feb 25, 2022
@gaearon gaearon mentioned this pull request Mar 29, 2022
zhengjitf pushed a commit to zhengjitf/react that referenced this pull request Apr 15, 2022
* Deprecate renderToNodeStream

* Use renderToPipeableStream in tests instead of renderToNodeStream

This is the equivalent API. This means that we have way less test coverage
of this API but I feel like that's fine since it has a deprecation warning
in it and we have coverage on renderToString that is mostly the same.

* Fix textarea bug

The test changes revealed a bug with textarea. It happens because we
currently always insert trailing comment nodes. We should optimize that
away. However, we also don't really support complex children so we
should toString it anyway which is what partial renderer used to do.

* Update tests that assert number of nodes

These tests are unnecessarily specific about number of nodes.

I special case these, which these tests already do, because they're good
tests to test that the optimization actually works later when we do
fix it.
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