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.

Sign up for GitHub

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

test: Update attribute fixture snapshot #24083

Merged
merged 4 commits into from
Apr 5, 2022
Merged

test: Update attribute fixture snapshot #24083

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b328849
test: Update attribute fixture snapshot
eps1lon Mar 12, 2022
932c0c4
Poke CircleCI
eps1lon Mar 12, 2022
46d7097
Merge branch 'main' into test/update-attribute-fixture
eps1lon Mar 13, 2022
3d1d243
Poke CircleCI
eps1lon Mar 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions fixtures/attribute-behavior/AttributeTableSnapshot.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9787,14 +9787,14 @@
| `src=(integer)`| (changed)| `"http://localhost:3000/1"` |
| `src=(NaN)`| (changed, warning)| `"http://localhost:3000/NaN"` |
| `src=(float)`| (changed)| `"http://localhost:3000/99.99"` |
| `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"` |
Comment on lines -9790 to +9797
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caused by #23316? /cc @gnoff

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably fine. Basically, all these changes are saying that we'll patch up these attributes on hydration instead of leaving them alone.

| `src=(null)`| (initial)| `<empty string>` |
| `src=(undefined)`| (initial)| `<empty string>` |

Expand Down Expand Up @@ -11980,21 +11980,21 @@
| `value=(empty string)`| (initial)| `<empty string>` |
| `value=(array with string)`| (changed)| `"string"` |
| `value=(empty array)`| (initial)| `<empty string>` |
| `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"` |
Comment on lines -11983 to +11991
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine

| `value=(string 'true')`| (changed)| `"true"` |
| `value=(string 'false')`| (changed)| `"false"` |
| `value=(string 'on')`| (changed)| `"on"` |
| `value=(string 'off')`| (changed)| `"off"` |
| `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>` |
Comment on lines -11996 to +11997
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be fixed? #24077 fixes these ssr mismatches for <select value />. Can we leverage #24077 to fix this new ssr mismatch as well as the (existing) ssr mismatch for <option value={symbol} />?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What tag is this on? The table doesn't say.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@sebmarkbage sebmarkbage Mar 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

| `value=(null)`| (initial, warning)| `<empty string>` |
| `value=(undefined)`| (initial)| `<empty string>` |

Expand Down