-
Notifications
You must be signed in to change notification settings - Fork 47.9k
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
Update attribute-behavior fixture #22522
Update attribute-behavior fixture #22522
Conversation
Comparing: 4e6eec6...293f9e8 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: (No significant changes) |
…vior/update-fixture
@@ -11918,7 +11968,7 @@ | |||
| `value=(string 'false')`| (changed)| `"false"` | | |||
| `value=(string 'on')`| (changed)| `"on"` | | |||
| `value=(string 'off')`| (changed)| `"off"` | | |||
| `value=(symbol)`| (initial, warning)| `<empty string>` | | |||
| `value=(symbol)`| (initial, warning, ssr error, ssr mismatch)| `<empty string>` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Warning on client and server in React 17: https://codesandbox.io/s/react-17-option-value-symbol-cy2rf
Throws on server in React 18 but also has an additional warning: https://codesandbox.io/s/react-18-option-value-symbol-kbc2n
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. Adding to TODOs to ensure this is intentional.
| `selected=(string 'on')`| (initial, warning, ssr warning)| `<boolean: true>` | | ||
| `selected=(string 'off')`| (initial, warning, ssr warning)| `<boolean: true>` | | ||
| `selected=(string 'on')`| (initial, warning)| `<boolean: true>` | | ||
| `selected=(string 'off')`| (initial, warning)| `<boolean: true>` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
React 17 only had a warning on the client: https://codesandbox.io/s/react-17-option-selected-on-5nl1q
React 18 has a warning on client and server: https://codesandbox.io/s/react-18-option-selected-on-9ciud
I think the "ssr warning" is a bit misleading here in my opinion. If the snapshot reads "ssr warning" I would expect a warning on the server. But the opposite is the case here.
Assuming that the other "ssr warning" changes follow the same pattern so not investigating those further
Just to clarify, does this mean the browser behavior changed in the meantime for these other changes? |
Based on the assumption that the last time the file was changed, I committed all changes and not just the ones for |
@@ -11880,22 +11930,22 @@ | |||
| `value=(empty string)`| (initial)| `<empty string>` | | |||
| `value=(array with string)`| (changed)| `"string"` | | |||
| `value=(empty array)`| (initial)| `<empty string>` | | |||
| `value=(object)`| (changed)| `"result of toString()"` | | |||
| `value=(object)`| (changed, ssr error, ssr mismatch)| `"result of toString()"` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a new throw. I'll add this to a list of things to check before the release.
@@ -2593,9 +2593,9 @@ | |||
| `defaultValue=(string 'false')`| (changed)| `"false"` | | |||
| `defaultValue=(string 'on')`| (changed)| `"on"` | | |||
| `defaultValue=(string 'off')`| (changed)| `"off"` | | |||
| `defaultValue=(symbol)`| (initial, ssr warning)| `<empty string>` | | |||
| `defaultValue=(function)`| (initial, ssr warning)| `<empty string>` | | |||
| `defaultValue=(null)`| (initial, ssr warning)| `<empty string>` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looked at this one. It's because old SSR always assigned this prop internally and so it went through the value
validation which complains about null
. Seems fine that it doesn't do that now.
@@ -526,23 +526,23 @@ | |||
## `amplitude` (on `<feFuncA>` inside `<svg>`) | |||
| Test Case | Flags | Result | | |||
| --- | --- | --- | | |||
| `amplitude=(string)`| (changed)| `<number: 0>` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looked at this one. Appears to be a browser behavior change. Chrome warns in console about invalid value.
@@ -10678,7 +10728,7 @@ | |||
| --- | --- | --- | | |||
| `style=(string)`| (changed, error, warning, ssr error)| `` | | |||
| `style=(empty string)`| (changed, error, warning, ssr error)| `` | | |||
| `style=(array with string)`| (initial)| `[]` | | |||
| `style=(array with string)`| (changed, error, warning, ssr mismatch)| `` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a browser behavior change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I've added the suspicious ones to pre-release checklist to follow up on.
* Fix missing key warning * Add build instructions * Update interpretation now that React 17 is latest stable and 18 is next * Ignore ReactDOM.render deprecation warning * Ensure a server implementation with `renderToString` is used * Update AttributeTableSnapshot * Ensure Popover doesn't overflow
Summary
Notable changes:
Wanted to add
imageSizes
andimageSrcSet
and noticed that theattribute-behavior
fixture is outdated.Fixture will still use the same APIs for 17 and next. Instead of changing APIs we just work around legacy warnings/errors.
Individual commits describe changes in more detail.
How did you test this change?
Followed (updated) setup instructions of
fixtures/attribute-behavior/README.md
usingnode@12.22.6
and Chrome Version 96.0.4664.45Why
AttributeTableSnapshot
changed?When creating a snapshot using the last commit where we updated the snapshot (feb134c), we get the following diff:
Changes on feb134c
If we subtract that diff from the diff of this PR we're left with the following changes:
I annotated those diffs with minimal repros.