Skip to content

Commit 343a45f

Browse files
authored
Remove initOption special case (#26595)
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.
1 parent 58742c2 commit 343a45f

File tree

3 files changed

+19
-21
lines changed

3 files changed

+19
-21
lines changed

packages/react-dom-bindings/src/client/ReactDOMComponent.js

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ import {
3232
updateInput,
3333
restoreControlledInputState,
3434
} from './ReactDOMInput';
35-
import {initOption, validateOptionProps} from './ReactDOMOption';
35+
import {validateOptionProps} from './ReactDOMOption';
3636
import {
3737
validateSelectProps,
3838
initSelect,
@@ -995,7 +995,6 @@ export function setInitialProperties(
995995
}
996996
}
997997
}
998-
initOption(domElement, props);
999998
return;
1000999
}
10011000
case 'dialog': {

packages/react-dom-bindings/src/client/ReactDOMOption.js

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88
*/
99

1010
import {Children} from 'react';
11-
import {getToStringValue, toString} from './ToStringValue';
1211

1312
let didWarnSelectedSetOnOption = false;
1413
let didWarnInvalidChild = false;
@@ -59,10 +58,3 @@ export function validateOptionProps(element: Element, props: Object) {
5958
}
6059
}
6160
}
62-
63-
export function initOption(element: Element, props: Object) {
64-
// value="" should make a value attribute (#6219)
65-
if (props.value != null) {
66-
element.setAttribute('value', toString(getToStringValue(props.value)));
67-
}
68-
}

packages/react-dom/src/__tests__/ReactDOMSelect-test.js

Lines changed: 18 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,13 @@
99

1010
'use strict';
1111

12+
// Fix JSDOM. setAttribute is supposed to throw on things that can't be implicitly toStringed.
13+
const setAttribute = Element.prototype.setAttribute;
14+
Element.prototype.setAttribute = function (name, value) {
15+
// eslint-disable-next-line react-internal/safe-string-coercion
16+
return setAttribute.call(this, name, '' + value);
17+
};
18+
1219
describe('ReactDOMSelect', () => {
1320
let React;
1421
let ReactDOM;
@@ -849,7 +856,7 @@ describe('ReactDOMSelect', () => {
849856
});
850857

851858
describe('When given a Symbol value', () => {
852-
it('treats initial Symbol value as an empty string', () => {
859+
it('treats initial Symbol value as missing', () => {
853860
let node;
854861

855862
expect(() => {
@@ -862,10 +869,10 @@ describe('ReactDOMSelect', () => {
862869
);
863870
}).toErrorDev('Invalid value for prop `value`');
864871

865-
expect(node.value).toBe('');
872+
expect(node.value).toBe('A Symbol!');
866873
});
867874

868-
it('treats updated Symbol value as an empty string', () => {
875+
it('treats updated Symbol value as missing', () => {
869876
let node;
870877

871878
expect(() => {
@@ -888,7 +895,7 @@ describe('ReactDOMSelect', () => {
888895
</select>,
889896
);
890897

891-
expect(node.value).toBe('');
898+
expect(node.value).toBe('A Symbol!');
892899
});
893900

894901
it('treats initial Symbol defaultValue as an empty string', () => {
@@ -904,7 +911,7 @@ describe('ReactDOMSelect', () => {
904911
);
905912
}).toErrorDev('Invalid value for prop `value`');
906913

907-
expect(node.value).toBe('');
914+
expect(node.value).toBe('A Symbol!');
908915
});
909916

910917
it('treats updated Symbol defaultValue as an empty string', () => {
@@ -930,12 +937,12 @@ describe('ReactDOMSelect', () => {
930937
</select>,
931938
);
932939

933-
expect(node.value).toBe('');
940+
expect(node.value).toBe('A Symbol!');
934941
});
935942
});
936943

937944
describe('When given a function value', () => {
938-
it('treats initial function value as an empty string', () => {
945+
it('treats initial function value as missing', () => {
939946
let node;
940947

941948
expect(() => {
@@ -948,7 +955,7 @@ describe('ReactDOMSelect', () => {
948955
);
949956
}).toErrorDev('Invalid value for prop `value`');
950957

951-
expect(node.value).toBe('');
958+
expect(node.value).toBe('A function!');
952959
});
953960

954961
it('treats initial function defaultValue as an empty string', () => {
@@ -964,7 +971,7 @@ describe('ReactDOMSelect', () => {
964971
);
965972
}).toErrorDev('Invalid value for prop `value`');
966973

967-
expect(node.value).toBe('');
974+
expect(node.value).toBe('A function!');
968975
});
969976

970977
it('treats updated function value as an empty string', () => {
@@ -990,7 +997,7 @@ describe('ReactDOMSelect', () => {
990997
</select>,
991998
);
992999

993-
expect(node.value).toBe('');
1000+
expect(node.value).toBe('A function!');
9941001
});
9951002

9961003
it('treats updated function defaultValue as an empty string', () => {
@@ -1016,7 +1023,7 @@ describe('ReactDOMSelect', () => {
10161023
</select>,
10171024
);
10181025

1019-
expect(node.value).toBe('');
1026+
expect(node.value).toBe('A function!');
10201027
});
10211028
});
10221029

0 commit comments

Comments
 (0)