Skip to content

Properly warn on Symbol/Function in SSR, eliminate defaultValue and defaultChecked reserved words, fix some SSR mismatches #13394

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

Closed
wants to merge 10 commits into from
Closed

Properly warn on Symbol/Function in SSR, eliminate defaultValue and defaultChecked reserved words, fix some SSR mismatches #13394

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
a6f0948
Update attribute-table
nhunzaker Aug 14, 2018
5015f96
Revert some test updates from master SSR tests
nhunzaker Sep 8, 2018
257293d
Revert arrow to function change in Select SSR test
nhunzaker Sep 8, 2018
a8ea1b9
Eliminate toString usage on selects
nhunzaker Sep 10, 2018
5bed9b0
Ensure multiple checked works in both directions
nhunzaker Sep 10, 2018
53e5033
Remove all toString usage in client select code
nhunzaker Sep 10, 2018
d094bdb
Remove toString usage in reactdominput
nhunzaker Sep 10, 2018
fbd1653
Remove toString usage in ReactDOMOption
nhunzaker Sep 10, 2018
26ef90b
Remove toString usage from textarea
nhunzaker Sep 10, 2018
da338d3
Remove string comparison and stringification from toStringValue
nhunzaker Sep 10, 2018
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
36 changes: 18 additions & 18 deletions fixtures/attribute-behavior/AttributeTableSnapshot.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2560,16 +2560,16 @@
| `defaultChecked=(-1)`| (changed)| `<boolean: true>` |
| `defaultChecked=(0)`| (initial)| `<boolean: false>` |
| `defaultChecked=(integer)`| (changed)| `<boolean: true>` |
| `defaultChecked=(NaN)`| (initial, warning, ssr warning)| `<boolean: false>` |
| `defaultChecked=(NaN)`| (initial, warning)| `<boolean: false>` |
| `defaultChecked=(float)`| (changed)| `<boolean: true>` |
| `defaultChecked=(true)`| (changed)| `<boolean: true>` |
| `defaultChecked=(false)`| (initial)| `<boolean: false>` |
| `defaultChecked=(string 'true')`| (changed)| `<boolean: true>` |
| `defaultChecked=(string 'false')`| (changed)| `<boolean: true>` |
| `defaultChecked=(string 'true')`| (changed, warning)| `<boolean: true>` |
| `defaultChecked=(string 'false')`| (changed, warning)| `<boolean: true>` |
| `defaultChecked=(string 'on')`| (changed)| `<boolean: true>` |
| `defaultChecked=(string 'off')`| (changed)| `<boolean: true>` |
| `defaultChecked=(symbol)`| (changed, ssr mismatch)| `<boolean: true>` |
| `defaultChecked=(function)`| (changed, ssr mismatch)| `<boolean: true>` |
| `defaultChecked=(symbol)`| (changed, warning, ssr mismatch)| `<boolean: true>` |
| `defaultChecked=(function)`| (changed, warning, ssr mismatch)| `<boolean: true>` |
| `defaultChecked=(null)`| (initial)| `<boolean: false>` |
| `defaultChecked=(undefined)`| (initial)| `<boolean: false>` |

Expand All @@ -2593,9 +2593,9 @@
| `defaultValue=(string 'false')`| (changed)| `"false"` |
| `defaultValue=(string 'on')`| (changed)| `"on"` |
| `defaultValue=(string 'off')`| (changed)| `"off"` |
| `defaultValue=(symbol)`| (initial, ssr warning)| `<empty string>` |
| `defaultValue=(function)`| (initial, ssr warning)| `<empty string>` |
| `defaultValue=(null)`| (initial, ssr warning)| `<empty string>` |
| `defaultValue=(symbol)`| (initial, warning)| `<empty string>` |
| `defaultValue=(function)`| (initial, warning)| `<empty string>` |
| `defaultValue=(null)`| (initial)| `<empty string>` |
| `defaultValue=(undefined)`| (initial)| `<empty string>` |

## `defaultValuE` (on `<input>` inside `<div>`)
Expand Down Expand Up @@ -11785,17 +11785,17 @@
| `value=(-1)`| (changed)| `"-1"` |
| `value=(0)`| (changed)| `"0"` |
| `value=(integer)`| (changed)| `"1"` |
| `value=(NaN)`| (changed, warning, ssr warning)| `"NaN"` |
| `value=(NaN)`| (changed, warning)| `"NaN"` |
| `value=(float)`| (changed)| `"99.99"` |
| `value=(true)`| (changed)| `"true"` |
| `value=(false)`| (changed)| `"false"` |
| `value=(string 'true')`| (changed)| `"true"` |
| `value=(string 'false')`| (changed)| `"false"` |
| `value=(string 'on')`| (changed)| `"on"` |
| `value=(string 'off')`| (changed)| `"off"` |
| `value=(symbol)`| (initial, warning, ssr warning)| `<empty string>` |
| `value=(function)`| (initial, warning, ssr warning)| `<empty string>` |
| `value=(null)`| (initial, warning, ssr warning)| `<empty string>` |
| `value=(symbol)`| (initial, warning)| `<empty string>` |
| `value=(function)`| (initial, warning)| `<empty string>` |
| `value=(null)`| (initial, warning)| `<empty string>` |
| `value=(undefined)`| (initial)| `<empty string>` |

## `value` (on `<input>` inside `<div>`)
Expand All @@ -11820,7 +11820,7 @@
| `value=(string 'off')`| (changed)| `"off"` |
| `value=(symbol)`| (initial, warning)| `<empty string>` |
| `value=(function)`| (initial, warning)| `<empty string>` |
| `value=(null)`| (initial, warning, ssr warning)| `<empty string>` |
| `value=(null)`| (initial, warning)| `<empty string>` |
| `value=(undefined)`| (initial)| `<empty string>` |

## `value` (on `<input>` inside `<div>`)
Expand All @@ -11845,7 +11845,7 @@
| `value=(string 'off')`| (initial)| `<empty string>` |
| `value=(symbol)`| (initial, warning)| `<empty string>` |
| `value=(function)`| (initial, warning)| `<empty string>` |
| `value=(null)`| (initial, warning, ssr warning)| `<empty string>` |
| `value=(null)`| (initial, warning)| `<empty string>` |
| `value=(undefined)`| (initial)| `<empty string>` |

## `value` (on `<textarea>` inside `<div>`)
Expand All @@ -11860,17 +11860,17 @@
| `value=(-1)`| (changed)| `"-1"` |
| `value=(0)`| (changed)| `"0"` |
| `value=(integer)`| (changed)| `"1"` |
| `value=(NaN)`| (changed, warning, ssr warning)| `"NaN"` |
| `value=(NaN)`| (changed, warning)| `"NaN"` |
| `value=(float)`| (changed)| `"99.99"` |
| `value=(true)`| (changed)| `"true"` |
| `value=(false)`| (changed)| `"false"` |
| `value=(string 'true')`| (changed)| `"true"` |
| `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=(function)`| (initial, warning, ssr mismatch)| `<empty string>` |
| `value=(null)`| (initial, warning, ssr warning)| `<empty string>` |
| `value=(symbol)`| (initial, warning)| `<empty string>` |
| `value=(function)`| (initial, warning)| `<empty string>` |
| `value=(null)`| (initial, warning)| `<empty string>` |
| `value=(undefined)`| (initial)| `<empty string>` |

## `value` (on `<option>` inside `<select>`)
Expand Down
20 changes: 12 additions & 8 deletions packages/react-dom/src/__tests__/ReactDOMInput-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1799,22 +1799,24 @@ describe('ReactDOMInput', () => {
});

it('treats initial Symbol defaultValue as an empty string', function() {
ReactDOM.render(<input defaultValue={Symbol('foobar')} />, container);
expect(() =>
ReactDOM.render(<input defaultValue={Symbol('foobar')} />, container),
).toWarnDev('Invalid value for prop `defaultValue`');
const node = container.firstChild;

expect(node.value).toBe('');
expect(node.getAttribute('value')).toBe('');
// TODO: we should warn here.
});

it('treats updated Symbol defaultValue as an empty string', function() {
ReactDOM.render(<input defaultValue="foo" />, container);
ReactDOM.render(<input defaultValue={Symbol('foobar')} />, container);
expect(() =>
ReactDOM.render(<input defaultValue={Symbol('foobar')} />, container),
).toWarnDev('Invalid value for prop `defaultValue`');
const node = container.firstChild;

expect(node.value).toBe('foo');
expect(node.getAttribute('value')).toBe('');
// TODO: we should warn here.
});
});

Expand Down Expand Up @@ -1847,22 +1849,24 @@ describe('ReactDOMInput', () => {
});

it('treats initial function defaultValue as an empty string', function() {
ReactDOM.render(<input defaultValue={() => {}} />, container);
expect(() =>
ReactDOM.render(<input defaultValue={() => {}} />, container),
).toWarnDev('Invalid value for prop `defaultValue`');
const node = container.firstChild;

expect(node.value).toBe('');
expect(node.getAttribute('value')).toBe('');
// TODO: we should warn here.
});

it('treats updated function defaultValue as an empty string', function() {
ReactDOM.render(<input defaultValue="foo" />, container);
ReactDOM.render(<input defaultValue={() => {}} />, container);
expect(() =>
ReactDOM.render(<input defaultValue={() => {}} />, container),
).toWarnDev('Invalid value for prop `defaultValue`');
const node = container.firstChild;

expect(node.value).toBe('foo');
expect(node.getAttribute('value')).toBe('');
// TODO: we should warn here.
});
});

Expand Down
43 changes: 25 additions & 18 deletions packages/react-dom/src/__tests__/ReactDOMSelect-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -858,14 +858,15 @@ describe('ReactDOMSelect', () => {
expect(() => {
node = ReactTestUtils.renderIntoDocument(
<select defaultValue={Symbol('foobar')}>
<option value={Symbol('foobar')}>A Symbol!</option>
<option value="monkey">A monkey!</option>
<option value="">Empty String</option>
<option value="giraffe">A giraffe!</option>
</select>,
);
}).toWarnDev('Invalid value for prop `value`');
}).toWarnDev('Invalid value for prop `defaultValue`');

expect(node.value).toBe('');
expect(node.options[1].selected).toBe(true);
});

it('treats updated Symbol defaultValue as an empty string', () => {
Expand All @@ -883,15 +884,18 @@ describe('ReactDOMSelect', () => {

expect(node.value).toBe('monkey');

node = ReactTestUtils.renderIntoDocument(
<select defaultValue={Symbol('foobar')}>
<option value={Symbol('foobar')}>A Symbol!</option>
<option value="monkey">A monkey!</option>
<option value="giraffe">A giraffe!</option>
</select>,
);
expect(() => {
node = ReactTestUtils.renderIntoDocument(
<select defaultValue={Symbol('foobar')}>
<option value="monkey">A monkey!</option>
<option value="">Empty String</option>
<option value="giraffe">A giraffe!</option>
</select>,
);
}).toWarnDev('Invalid value for prop `defaultValue`');

expect(node.value).toBe('');
expect(node.options[1].selected).toBe(true);
});
});

Expand All @@ -918,14 +922,15 @@ describe('ReactDOMSelect', () => {
expect(() => {
node = ReactTestUtils.renderIntoDocument(
<select defaultValue={() => {}}>
<option value={() => {}}>A function!</option>
<option value="monkey">A monkey!</option>
<option value="">Empty String</option>
<option value="giraffe">A giraffe!</option>
</select>,
);
}).toWarnDev('Invalid value for prop `value`');
}).toWarnDev('Invalid value for prop `defaultValue`');

expect(node.value).toBe('');
expect(node.options[1].selected).toBe(true);
});

it('treats updated function value as an empty string', () => {
Expand Down Expand Up @@ -969,13 +974,15 @@ describe('ReactDOMSelect', () => {

expect(node.value).toBe('monkey');

node = ReactTestUtils.renderIntoDocument(
<select defaultValue={() => {}}>
<option value={() => {}}>A function!</option>
<option value="monkey">A monkey!</option>
<option value="giraffe">A giraffe!</option>
</select>,
);
expect(() => {
node = ReactTestUtils.renderIntoDocument(
<select defaultValue={() => {}}>
<option value={() => {}}>A function!</option>
<option value="monkey">A monkey!</option>
<option value="giraffe">A giraffe!</option>
</select>,
);
}).toWarnDev('Invalid value for prop `defaultValue`');

expect(node.value).toBe('');
});
Expand Down
106 changes: 106 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMServerIntegrationSelect-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,4 +247,110 @@ describe('ReactDOMServerIntegrationSelect', () => {
expect(e.firstChild.selected).toBe(true);
},
);

itRenders('a select with Symbol value with a warning', async render => {
const e = await render(
<select value={Symbol('test')} readOnly={true} />,
1,
);
expect(e.hasAttribute('value')).toBe(false);
});

itRenders('an option with Symbol value with a warning', async render => {
const e = await render(
<select readOnly={true} value={Symbol('test')}>
<option value={Symbol('test')} />
</select>,
1,
);
expect(e.value).toBe('');
expect(e.firstChild.value).toBe('');
});

itRenders('a select with NaN value with a warning', async render => {
const e = await render(
<select value={NaN} readOnly={true}>
<option value="NaN">NaN</option>
</select>,
1,
);
expect(e.value).toBe('NaN');
});

itRenders('a select with NaN defaultValue with a warning', async render => {
const e = await render(
<select defaultValue={NaN} readOnly={true}>
<option value="NaN">NaN</option>
</select>,
1,
);
expect(e.value).toBe('NaN');
});

itRenders('a select with function value with a warning', async render => {
const e = await render(
<select value={() => {}} readOnly={true}>
<option>First option</option>
<option value={(() => {}).toString()} />
</select>,
1,
);
expect(e.value).toBe('First option');
});

itRenders(
'a select with function defaultValue with a warning',
async render => {
const e = await render(
<select defaultValue={() => {}} readOnly={true}>
<option>First option</option>
<option value={(() => {}).toString()} />
</select>,
1,
);
expect(e.value).toBe('First option');
},
);

itRenders(
'selects the correct option even with an option type mismatch',
async render => {
const e = await render(
<select value={3} readOnly={true}>
<option>None</option>
<option value="3">3</option>
</select>,
);
expect(e.options[1].selected).toBe(true);
},
);

itRenders(
'selects the correct option even with a select type mismatch',
async render => {
const e = await render(
<select value="3" readOnly={true}>
<option>None</option>
<option value={3}>3</option>
</select>,
);
expect(e.options[1].selected).toBe(true);
},
);

itRenders(
'multiselects the correct options even with a type mismatch',
async render => {
const e = await render(
<select multiple={true} value={[1, '3']} readOnly={true}>
<option>None</option>
<option value="1">1</option>
<option value={3}>3</option>
</select>,
);
expect(e.options[0].selected).toBe(false);
expect(e.options[1].selected).toBe(true);
expect(e.options[2].selected).toBe(true);
},
);
});
Loading