Skip to content

Conversation

@sebmarkbage
Copy link
Collaborator

This is the source of the difference in symbol throwing behavior noted in #22522 (comment)

We always did throw for symbols when a "value" is provided to the parent <select> component. It's just that I treated undefined as providing a value. However, we also didn't toString that value so if it's one of the other primitives it didn't match.

}

if (selectedValue !== null) {
if (selectedValue != null) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The type of selectedValue is a lie. We should probably toString it and resolve the undefined earlier.

@sizebot
Copy link

sizebot commented Mar 11, 2022

Comparing: 581f0c4...97b6ad5

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 = 130.94 kB 130.94 kB = 41.92 kB 41.92 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 136.01 kB 136.01 kB = 43.40 kB 43.40 kB
facebook-www/ReactDOM-prod.classic.js = 435.49 kB 435.49 kB = 79.78 kB 79.78 kB
facebook-www/ReactDOM-prod.modern.js = 421.91 kB 421.91 kB = 77.76 kB 77.76 kB
facebook-www/ReactDOMForked-prod.classic.js = 435.49 kB 435.49 kB = 79.78 kB 79.78 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 97b6ad5

Copy link
Collaborator

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

This also updates the attribute fixture:

diff --git a/fixtures/attribute-behavior/AttributeTableSnapshot.md b/fixtures/attribute-behavior/AttributeTableSnapshot.md
index c3a59c1f81..52a6bc31ab 100644
--- a/fixtures/attribute-behavior/AttributeTableSnapshot.md
+++ b/fixtures/attribute-behavior/AttributeTableSnapshot.md
@@ -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>` |
 | `value=(function)`| (initial, warning)| `<empty string>` |
 | `value=(null)`| (initial)| `<empty string>` |
 | `value=(undefined)`| (initial)| `<empty string>` |

There are some other changes in there as well if you update it on this branch that are already on main. Changes for main are included in #24083

@sebmarkbage sebmarkbage merged commit 7967240 into facebook:main Mar 12, 2022
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