Skip to content

Enable warnings for the defaultValue prop #13360

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 5 commits into from
Closed

Enable warnings for the defaultValue prop #13360

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8de05f8
tests: remove TODOs about warnings in ReactDOMInput-test.js
raunofreiberg Aug 10, 2018
e1aeb5c
fix: remove fit
raunofreiberg Aug 10, 2018
4d22d81
Run prettier
raunofreiberg Aug 10, 2018
5120ce3
refactor: Enable warnings for the defaultValue prop in shouldRemoveAt…
raunofreiberg Aug 10, 2018
2132b74
refactor: use shouldWarnInDev boolean for reserved props warnings
raunofreiberg Aug 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
31 changes: 21 additions & 10 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 @@ -400,14 +400,20 @@ describe('ReactDOMInput', () => {

it('should display "true" for `defaultValue` of `true`', () => {
let stub = <input type="text" defaultValue={true} />;
const node = ReactDOM.render(stub, container);
let node;
expect(() => (node = ReactDOM.render(stub, container))).toWarnDev(
'Received `true` for a non-boolean attribute `defaultValue`',
);

expect(node.value).toBe('true');
});

it('should display "false" for `defaultValue` of `false`', () => {
let stub = <input type="text" defaultValue={false} />;
const node = ReactDOM.render(stub, container);
let node;
expect(() => (node = ReactDOM.render(stub, container))).toWarnDev(
'Received `false` for a non-boolean attribute `defaultValue`',
);

expect(node.value).toBe('false');
});
Expand Down Expand Up @@ -1641,22 +1647,25 @@ 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 @@ -1689,22 +1698,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
5 changes: 4 additions & 1 deletion packages/react-dom/src/__tests__/ReactDOMTextarea-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,10 @@ describe('ReactDOMTextarea', () => {

it('should display "false" for `defaultValue` of `false`', () => {
const stub = <textarea defaultValue={false} />;
const node = renderTextarea(stub);
let node;
expect(() => (node = renderTextarea(stub))).toWarnDev(
'Received `false` for a non-boolean attribute `defaultValue`',
);

expect(node.value).toBe('false');
});
Expand Down
12 changes: 11 additions & 1 deletion packages/react-dom/src/shared/DOMProperty.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ export type PropertyInfo = {|
+mustUseProperty: boolean,
+propertyName: string,
+type: PropertyType,
+shouldWarnInDev: boolean,
|};

/* eslint-disable max-len */
Expand Down Expand Up @@ -115,7 +116,13 @@ export function shouldRemoveAttributeWithWarning(
propertyInfo: PropertyInfo | null,
isCustomComponentTag: boolean,
): boolean {
if (propertyInfo !== null && propertyInfo.type === RESERVED) {
// Do not validate data types for reserved props
// (excluding defaultValue)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a bit odd. Maybe we shouldn’t treat it as reserved?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, but wouldn't we then be committing unwanted attributes to the DOM?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don’t know. You tell me what would happen :-)

Copy link
Contributor Author

@raunofreiberg raunofreiberg Aug 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I went ahead and uncommented defaultValue from the list of reserved properties and a few test suites started to fail in a similar manner (for e.g, ReactDOMServerIntegrationForms), confirming what I was expecting.

E.g.

image

This seems to only affect SSR, client side cases seem to pass, for e.g:

const div = document.createElement('div');
const node = ReactDOM.render(<textarea defaultValue="1" />, div);
expect(node.getAttribute('defaultValue')).toBe(null);

I'm not too certain nor familiar with how strict we want to be about reserved properties (committing them to the DOM is definitely out of the question, I would assume).

Maybe we can keep the attribute reserved as is, but instead add another key to PropertyInfo (e.g. boolean shouldWarnInDev) to enable warnings for custom properties. This way the condition that you mentioned above would become:

if (
  propertyInfo !== null &&
  propertyInfo.type === RESERVED &&
  !propertyInfo.shouldWarnInDev
) {
  return false;
}

Also, enabling warnings for other reserved properties would be a breeze this way, IMHO :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a proof of concept commit to illustrate my approach better.

if (
propertyInfo !== null &&
propertyInfo.type === RESERVED &&
!propertyInfo.shouldWarnInDev
) {
return false;
}
switch (typeof value) {
Expand Down Expand Up @@ -186,6 +193,7 @@ function PropertyInfoRecord(
mustUseProperty: boolean,
attributeName: string,
attributeNamespace: string | null,
shouldWarnInDev: boolean = false,
) {
this.acceptsBooleans =
type === BOOLEANISH_STRING ||
Expand All @@ -196,6 +204,7 @@ function PropertyInfoRecord(
this.mustUseProperty = mustUseProperty;
this.propertyName = name;
this.type = type;
this.shouldWarnInDev = shouldWarnInDev;
}

// When adding attributes to this list, be sure to also add them to
Expand Down Expand Up @@ -223,6 +232,7 @@ const properties = {};
false, // mustUseProperty
name, // attributeName
null, // attributeNamespace
name === 'defaultValue', // shouldWarnInDev
);
});

Expand Down
6 changes: 0 additions & 6 deletions packages/react-dom/src/shared/ReactDOMUnknownPropertyHook.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,12 +214,6 @@ if (__DEV__) {
return true;
}

// Now that we've validated casing, do not validate
// data types for reserved props
if (isReserved) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to remove this since the shouldRemoveAttributeWithWarning function already duplicates this logic internally.

return true;
}

// Warn when a known attribute is a bad type
if (shouldRemoveAttributeWithWarning(name, value, propertyInfo, false)) {
warnedProperties[name] = true;
Expand Down