Skip to content

Commit

Permalink
Remove initOption special case (facebook#26595)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
sebmarkbage authored Apr 11, 2023
1 parent 58742c2 commit 343a45f
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 21 deletions.
3 changes: 1 addition & 2 deletions packages/react-dom-bindings/src/client/ReactDOMComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ import {
updateInput,
restoreControlledInputState,
} from './ReactDOMInput';
import {initOption, validateOptionProps} from './ReactDOMOption';
import {validateOptionProps} from './ReactDOMOption';
import {
validateSelectProps,
initSelect,
Expand Down Expand Up @@ -995,7 +995,6 @@ export function setInitialProperties(
}
}
}
initOption(domElement, props);
return;
}
case 'dialog': {
Expand Down
8 changes: 0 additions & 8 deletions packages/react-dom-bindings/src/client/ReactDOMOption.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
*/

import {Children} from 'react';
import {getToStringValue, toString} from './ToStringValue';

let didWarnSelectedSetOnOption = false;
let didWarnInvalidChild = false;
Expand Down Expand Up @@ -59,10 +58,3 @@ export function validateOptionProps(element: Element, props: Object) {
}
}
}

export function initOption(element: Element, props: Object) {
// value="" should make a value attribute (#6219)
if (props.value != null) {
element.setAttribute('value', toString(getToStringValue(props.value)));
}
}
29 changes: 18 additions & 11 deletions packages/react-dom/src/__tests__/ReactDOMSelect-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,13 @@

'use strict';

// Fix JSDOM. setAttribute is supposed to throw on things that can't be implicitly toStringed.
const setAttribute = Element.prototype.setAttribute;
Element.prototype.setAttribute = function (name, value) {
// eslint-disable-next-line react-internal/safe-string-coercion
return setAttribute.call(this, name, '' + value);
};

describe('ReactDOMSelect', () => {
let React;
let ReactDOM;
Expand Down Expand Up @@ -849,7 +856,7 @@ describe('ReactDOMSelect', () => {
});

describe('When given a Symbol value', () => {
it('treats initial Symbol value as an empty string', () => {
it('treats initial Symbol value as missing', () => {
let node;

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

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

it('treats updated Symbol value as an empty string', () => {
it('treats updated Symbol value as missing', () => {
let node;

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

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

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

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

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

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

describe('When given a function value', () => {
it('treats initial function value as an empty string', () => {
it('treats initial function value as missing', () => {
let node;

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

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

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

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

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

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

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

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

Expand Down

0 comments on commit 343a45f

Please sign in to comment.