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.

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

Remove initOption special case #26595

Merged
merged 1 commit into from
Apr 11, 2023
Merged

Conversation

sebmarkbage
Copy link
Collaborator

@sebmarkbage sebmarkbage commented Apr 11, 2023

This traces back to #6449 and then another before that.

I think that back then we favored the property over the attribute, and setting the property wouldn't be enough. However, the default path for these are now using attributes if we don't special case it. So we don't need it.

The only difference is that we currently have a divergence for symbol/function behavior between controlled values that use the getToStringValue helpers which treat them as empty string, where as everywhere else they're treated as null/missing.

Since this comes with a warning and is a weird error case, it's probably fine to change.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Apr 11, 2023
@react-sizebot
Copy link

react-sizebot commented Apr 11, 2023

Comparing: ac43bf6...9159240

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 = 164.61 kB 164.55 kB = 51.69 kB 51.67 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 167.00 kB 166.94 kB = 52.31 kB 52.29 kB
facebook-www/ReactDOM-prod.classic.js = 565.24 kB 565.13 kB = 99.34 kB 99.31 kB
facebook-www/ReactDOM-prod.modern.js = 549.08 kB 548.98 kB = 96.66 kB 96.63 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 9159240

@@ -888,7 +888,7 @@ describe('ReactDOMSelect', () => {
</select>,
);

expect(node.value).toBe('');
expect(node.value).toBe('A Symbol!');
});

it('treats initial Symbol defaultValue as an empty string', () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These defaultValue tests are bad tests because they test what the current selection is with the first option, but since the first option is selected when there is no match, they don't fail if it just doesn't match.

@@ -9,6 +9,12 @@

'use strict';

// Fix JSDOM. setAttribute is supposed to throw on things that can't be implicitly toStringed.
Copy link
Collaborator Author

@sebmarkbage sebmarkbage Apr 11, 2023

Choose a reason for hiding this comment

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

This is a little hacky but it's making the TemporalLike objects throw without us explicitly tostringing in React which we shouldn't need to, since there can be Trusted Types involved. Maybe a newer JSDOM would help.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we unset this in an afterAll so it's just for this file? Or move it to setupEnvironment.js (or setupTests.js? I don't know what the difference is)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Isn't it a new document for every test suite anyway? I think we rely on that anyway.

I wanted to colocate this fix so that we can choose to fix the tests instead if we need to.

@sebmarkbage sebmarkbage force-pushed the rminitoption branch 2 times, most recently from 85bfc2a to f5824b9 Compare April 11, 2023 03:28
@@ -9,6 +9,12 @@

'use strict';

// Fix JSDOM. setAttribute is supposed to throw on things that can't be implicitly toStringed.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we unset this in an afterAll so it's just for this file? Or move it to setupEnvironment.js (or setupTests.js? I don't know what the difference is)?

@sebmarkbage sebmarkbage merged commit 343a45f into facebook:main Apr 11, 2023
github-actions bot pushed a commit that referenced this pull request Apr 11, 2023
This traces back to #6449 and then
another before that.

I think that back then we favored the property over the attribute, and
setting the property wouldn't be enough. However, the default path for
these are now using attributes if we don't special case it. So we don't
need it.

The only difference is that we currently have a divergence for
symbol/function behavior between controlled values that use the
getToStringValue helpers which treat them as empty string, where as
everywhere else they're treated as null/missing.

Since this comes with a warning and is a weird error case, it's probably
fine to change.

DiffTrain build for [343a45f](343a45f)
kassens added a commit to kassens/react that referenced this pull request Apr 17, 2023
kassens pushed a commit to kassens/react that referenced this pull request Apr 21, 2023
, facebook#26595, facebook#26596, facebook#26627

- Refactor some controlled component stuff (facebook#26573)
- Don't update textarea defaultValue and input checked unnecessarily (facebook#26580)
- Diff properties in the commit phase instead of generating an update payload (facebook#26583)
- Move validation of text nesting into ReactDOMComponent  (facebook#26594)
- Remove initOption special case (facebook#26595)
- Use already extracted values instead of reading off props for controlled components (facebook#26596)
- Fix input tracking bug (facebook#26627)
github-actions bot pushed a commit that referenced this pull request Apr 21, 2023
- Refactor some controlled component stuff (#26573)
- Don't update textarea defaultValue and input checked unnecessarily (#26580)
- Diff properties in the commit phase instead of generating an update payload (#26583)
- Move validation of text nesting into ReactDOMComponent  (#26594)
- Remove initOption special case (#26595)
- Use already extracted values instead of reading off props for controlled components (#26596)
- Fix input tracking bug (#26627)

DiffTrain build for [ded4a78](ded4a78)
kassens pushed a commit that referenced this pull request Apr 21, 2023
- Refactor some controlled component stuff (#26573)
- Don't update textarea defaultValue and input checked unnecessarily (#26580)
- Diff properties in the commit phase instead of generating an update payload (#26583)
- Move validation of text nesting into ReactDOMComponent  (#26594)
- Remove initOption special case (#26595)
- Use already extracted values instead of reading off props for controlled components (#26596)
- Fix input tracking bug (#26627)
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
This traces back to facebook#6449 and then
another before that.

I think that back then we favored the property over the attribute, and
setting the property wouldn't be enough. However, the default path for
these are now using attributes if we don't special case it. So we don't
need it.

The only difference is that we currently have a divergence for
symbol/function behavior between controlled values that use the
getToStringValue helpers which treat them as empty string, where as
everywhere else they're treated as null/missing.

Since this comes with a warning and is a weird error case, it's probably
fine to change.
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
This traces back to #6449 and then
another before that.

I think that back then we favored the property over the attribute, and
setting the property wouldn't be enough. However, the default path for
these are now using attributes if we don't special case it. So we don't
need it.

The only difference is that we currently have a divergence for
symbol/function behavior between controlled values that use the
getToStringValue helpers which treat them as empty string, where as
everywhere else they're treated as null/missing.

Since this comes with a warning and is a weird error case, it's probably
fine to change.

DiffTrain build for commit 343a45f.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants