test: Update attribute fixture snapshot #24083
Conversation
|
Comparing: 061ac27...3d1d243 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) |
| | `src=(true)`| (initial, warning)| `<empty string>` | | ||
| | `src=(true)`| (changed, warning, ssr mismatch)| `"http://localhost:3000/true"` | | ||
| | `src=(false)`| (initial, warning)| `<empty string>` | | ||
| | `src=(string 'true')`| (changed)| `"http://localhost:3000/true"` | | ||
| | `src=(string 'false')`| (changed)| `"http://localhost:3000/false"` | | ||
| | `src=(string 'on')`| (changed)| `"http://localhost:3000/on"` | | ||
| | `src=(string 'off')`| (changed)| `"http://localhost:3000/off"` | | ||
| | `src=(symbol)`| (initial, warning)| `<empty string>` | | ||
| | `src=(function)`| (initial, warning)| `<empty string>` | | ||
| | `src=(symbol)`| (changed, error, warning, ssr mismatch)| `` | | ||
| | `src=(function)`| (changed, warning, ssr mismatch)| `"http://localhost:3000/function%20f()%20%7B%7D"` | |
There was a problem hiding this comment.
This is probably fine. Basically, all these changes are saying that we'll patch up these attributes on hydration instead of leaving them alone.
| | `value=(object)`| (changed, ssr error, ssr mismatch)| `"result of toString()"` | | ||
| | `value=(object)`| (changed)| `"result of toString()"` | | ||
| | `value=(numeric string)`| (changed)| `"42"` | | ||
| | `value=(-1)`| (changed)| `"-1"` | | ||
| | `value=(0)`| (changed)| `"0"` | | ||
| | `value=(integer)`| (changed)| `"1"` | | ||
| | `value=(NaN)`| (changed, warning)| `"NaN"` | | ||
| | `value=(float)`| (changed)| `"99.99"` | | ||
| | `value=(true)`| (changed, ssr mismatch)| `"true"` | | ||
| | `value=(false)`| (changed, ssr mismatch)| `"false"` | | ||
| | `value=(true)`| (changed)| `"true"` | | ||
| | `value=(false)`| (changed)| `"false"` | |
| | `value=(symbol)`| (initial, warning)| `<empty string>` | | ||
| | `value=(function)`| (initial, warning)| `<empty string>` | | ||
| | `value=(symbol)`| (initial, warning, ssr error, ssr mismatch)| `<empty string>` | | ||
| | `value=(function)`| (initial, warning, ssr mismatch)| `<empty string>` | |
There was a problem hiding this comment.
What tag is this on? The table doesn't say.
There was a problem hiding this comment.
One thing that's tricky is where we do these conversions. Should they be done by the select tag so that if you provide no option, it should happen anyway. Or should they be done by the option tag when the comparison actually happens.
It probably makes more sense to do them on the select and eagerly convert them to a cloned array of strings.
There was a problem hiding this comment.
What tag is this on? The table doesn't say.
The expand feature on github.com is broken here. This line is under the "value (on <textarea> inside <div>)" section: https://github.dev/facebook/react/pull/24083#pullrequestreview-909231364
There was a problem hiding this comment.
I see so the change happened in e915eee
The thing is that the fix in #24077 kind of relies on us only toStringing if one if the sides is not defined. I think that the proper fix is to always toString if a value is provided (and always doing so inside the component it is provided so in the select instead of the option for selected value). In other words, if we fix that then maybe we'd actually revert to erroring again.
So I think that it's correct that this errors.
The bug is that the textarea on the client doesn't error by going through getToStringValue. In other words, I think we should revert #13389 and #13362, which trace back to #11741.
My take on the history of this fix is that it was discovered but as Dan pointed out #11734 (comment) it was more consistent now. The original issue remained open and got "fixed" but it didn't need fixing.
We should definitely not silently accept Symbols. It should at least warn. But in general we favor throwing in prod too if it doesn't add perf regressions. In this case we're adding a perf regression to avoid throwing in prod, and cover up bugs.
There was a problem hiding this comment.
I guess we pretty pervasively check for symbols and silently ignore them for custom props too, which is not great. We should at least add warnings for all those cases and then change it along with other DOM breaking changes we have planned.
I'm not sure if it's worth fixing on the server case. Might as well sneak in a small breakage that we're planning on doing anyway.
| @@ -12018,7 +12018,7 @@ | |||
| | `value=(string 'false')`| (changed)| `"false"` | | |||
| | `value=(string 'on')`| (changed)| `"on"` | | |||
| | `value=(string 'off')`| (changed)| `"off"` | | |||
| | `value=(symbol)`| (initial, warning, ssr error, ssr mismatch)| `<empty string>` | | |||
| | `value=(symbol)`| (initial, warning)| `<empty string>` | | |||
* test: Update attribute fixture snapshot * Poke CircleCI * Poke CircleCI
* test: Update attribute fixture snapshot * Poke CircleCI * Poke CircleCI
* test: Update attribute fixture snapshot * Poke CircleCI * Poke CircleCI
Summary
Updates the attribute fixture snapshot.
How did you test this change?
yarn build --type=UMD_DEV react/index,react-dom && cd fixtures/attribute-behavior && yarn install && yarn startand "save latest results to a file" tofixtures/attribute-behavior/AttributeTableSnapshot.md