Fix and update attribute-behavior fixture #26114

eps1lon · 2023-02-06T11:38:57Z

Summary

Due to #25928 the attribute fixture could no longer finish since it expects at least something to render. But since Fizz currently breaks down completely on malformed <meta> tags, the fixture could no longer handle this.

The fixture now renders valid types for meta tags.

Note that the snapshot change to viewTarget`` is already on main`. Review by commit helps to understand this.

Added html[lang] so that we test at least one standard attribute on <html>. version is obsolete so results are not that trustworthy.

How did you test this change?

With Chrome Version 109.0.5414.119 (Official Build) (64-bit)

yarn build --type=UMD_DEV react/index,react-dom && cd fixtures/attribute-behavior && yarn install && yarn start

react-sizebot · 2023-02-06T11:46:47Z

Comparing: 55542bc...dfb5f26

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	=	154.56 kB	154.56 kB	=	48.84 kB	48.84 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	156.56 kB	156.56 kB	=	49.46 kB	49.46 kB
facebook-www/ReactDOM-prod.classic.js	=	529.69 kB	529.69 kB	=	94.36 kB	94.36 kB
facebook-www/ReactDOM-prod.modern.js	=	514.91 kB	514.91 kB	=	92.16 kB	92.16 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against dfb5f26

fixtures/attribute-behavior/AttributeTableSnapshot.md

gnoff · 2023-02-08T19:09:27Z

@eps1lon once #26106 lands the meta tags will get hoisted but won't require the extra props to avoid getting dropped. do you want to wait for that to land this or undo these changes once that one is incorporated? Also is this running on oss-stable only? It seems it is not flagged but we could in theory have singletons and hoistables on for some builds and not others

eps1lon · 2023-02-08T21:14:10Z

@eps1lon once #26106 lands the meta tags will get hoisted but won't require the extra props to avoid getting dropped. do you want to wait for that to land this or undo these changes once that one is incorporated?

I think it's better to wait for #26106 for now.

Also is this running on oss-stable only?

It's running on the published latest and on main. But I think only main is actually used for the snapshot.

Ran without `<meta>` tags which currently crash the fixture

`version` is obsolete so I was hoping this result doesn't matter. But `lang` has the same behavior i.e. isn't added to the DOM on the client

eps1lon · 2023-02-10T13:32:34Z

fixtures/attribute-behavior/src/App.js

@@ -282,12 +284,12 @@ function getRenderedAttributeValue(
 try {
 let container = createContainer();
 renderer.render(react.createElement(tagName, baseProps), container);
- defaultValue = read(container.firstChild);
+ defaultValue = read(container.lastChild);


Consistent with how we read the value of server-rendered markup. firstChild returns the DOCTYPE now if we render into a document on the client.

fixtures/attribute-behavior/src/attributes.js

gnoff · 2023-02-10T14:57:23Z

fixtures/attribute-behavior/src/App.js

@@ -764,7 +772,7 @@ class App extends React.Component {
 ReactDOMStable:
 'https://unpkg.com/react-dom@latest/umd/react-dom.development.js',
 ReactDOMServerStable:
- 'https://unpkg.com/react-dom@latest/umd/react-dom-server.browser.development.js',
+ 'https://unpkg.com/react-dom@latest/umd/react-dom-server-legacy.browser.development.js',


What caused this change?

renderToString is no longer included in react-dom-server.browser. Makes sense considering renderToString is considered legacy. renderToString was already excluded from the 18.0.0 UMD bundle so there was no change to the UMD bundle in a SemVer minor.

oh odd, I thought that was already the case. sounds good

## Summary Due to #25928 the attribute fixture could no longer finish since it expects at least something to render. But since Fizz currently breaks down completely on malformed `<meta>` tags, the fixture could no longer handle this. The fixture now renders valid types for `meta` tags. Note that the snapshot change to `viewTarget`` is already on `main`. Review by commit helps to understand this. Added `html[lang]` so that we test at least one standard attribute on `<html>`. `version` is obsolete so results are not that trustworthy. ## How did you test this change? With Chrome Version 109.0.5414.119 (Official Build) (64-bit) - `yarn build --type=UMD_DEV react/index,react-dom && cd fixtures/attribute-behavior && yarn install && yarn start` DiffTrain build for [b8ae89f](b8ae89f) [View git log for this commit](https://github.com/facebook/react/commits/b8ae89f38288bfae37dff54fa1ec4bf3b4555ed5)

eps1lon requested a review from sebmarkbage February 6, 2023 11:38

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Feb 6, 2023

eps1lon commented Feb 8, 2023

View reviewed changes

fixtures/attribute-behavior/AttributeTableSnapshot.md Outdated Show resolved Hide resolved

eps1lon marked this pull request as draft February 8, 2023 21:14

Update attribute-behavior snapshot (except <meta>)

ab71467

Ran without `<meta>` tags which currently crash the fixture

eps1lon force-pushed the test/attribute-fixture-update branch from 1adecbe to 5f9e4ff Compare February 10, 2023 13:06

eps1lon changed the title ~~[attribute-fixture] Fix and update~~ Fix and update attribute-behavior fixture Feb 10, 2023

eps1lon added 4 commits February 10, 2023 14:30

Ensure Stable and Next have use same server impl

d0032ab

Render valid meta tags

0de4cde

Verify html[version] result by adding html[lang]

587810d

`version` is obsolete so I was hoping this result doesn't matter. But `lang` has the same behavior i.e. isn't added to the DOM on the client

Ensure we find the rendered element if we render into document

9a2f7f7

eps1lon force-pushed the test/attribute-fixture-update branch from 96dd1bf to 9a2f7f7 Compare February 10, 2023 13:31

eps1lon commented Feb 10, 2023

View reviewed changes

eps1lon requested a review from gnoff February 10, 2023 13:34

eps1lon marked this pull request as ready for review February 10, 2023 13:34

gnoff reviewed Feb 10, 2023

View reviewed changes

Remove unnecessary extraProps

dfb5f26

eps1lon requested a review from gnoff February 10, 2023 15:54

gnoff approved these changes Feb 10, 2023

View reviewed changes

eps1lon merged commit b8ae89f into facebook:main Feb 10, 2023

eps1lon deleted the test/attribute-fixture-update branch February 10, 2023 18:19

kassens mentioned this pull request Feb 10, 2023

Minor Jest upgrade kassens/react#3

Closed

sammy-SC mentioned this pull request Apr 5, 2023

Use native scheduler if defined sammy-SC/react#1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix and update attribute-behavior fixture #26114

Fix and update attribute-behavior fixture #26114

eps1lon commented Feb 6, 2023 •

edited

Loading

react-sizebot commented Feb 6, 2023 •

edited

Loading

gnoff commented Feb 8, 2023

eps1lon commented Feb 8, 2023

eps1lon Feb 10, 2023

gnoff Feb 10, 2023

eps1lon Feb 10, 2023

gnoff Feb 10, 2023

Fix and update attribute-behavior fixture #26114

Fix and update attribute-behavior fixture #26114

Conversation

eps1lon commented Feb 6, 2023 • edited Loading

Summary

How did you test this change?

react-sizebot commented Feb 6, 2023 • edited Loading

Critical size changes

Significant size changes

gnoff commented Feb 8, 2023

eps1lon commented Feb 8, 2023

eps1lon Feb 10, 2023

Choose a reason for hiding this comment

gnoff Feb 10, 2023

Choose a reason for hiding this comment

eps1lon Feb 10, 2023

Choose a reason for hiding this comment

gnoff Feb 10, 2023

Choose a reason for hiding this comment

eps1lon commented Feb 6, 2023 •

edited

Loading

react-sizebot commented Feb 6, 2023 •

edited

Loading