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.

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

Warn against custom non-lowercase attributes #10699

Merged
merged 3 commits into from
Sep 13, 2017
Merged

Warn against custom non-lowercase attributes #10699

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0f271e5
Warn against custom non-lowercase attributes
gaearon Sep 13, 2017
89584aa
Update attribute table
gaearon Sep 13, 2017
07073ea
Grammar tweaks
gaearon Sep 13, 2017
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
122 changes: 61 additions & 61 deletions fixtures/attribute-behavior/AttributeTableSnapshot.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -701,23 +701,23 @@
## `as` (on `<div>`)
| Test Case | Flags | Result |
| --- | --- | --- |
| `as=(string)`| (changed)| `"a string"` |
| `as=(string)`| (initial)| `<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.

This is an unrelated change, and seems like Chrome version difference. Setting <link as="whatever"> doesn't return "whatever" from link.as property anymore. But it works with valid values like "audio".

| `as=(empty string)`| (initial)| `<empty string>` |
| `as=(array with string)`| (changed)| `"string"` |
| `as=(array with string)`| (initial)| `<empty string>` |
| `as=(empty array)`| (initial)| `<empty string>` |
| `as=(object)`| (changed)| `"result of toString()"` |
| `as=(numeric string)`| (changed)| `"42"` |
| `as=(-1)`| (changed)| `"-1"` |
| `as=(0)`| (changed)| `"0"` |
| `as=(integer)`| (changed)| `"1"` |
| `as=(NaN)`| (changed, warning)| `"NaN"` |
| `as=(float)`| (changed)| `"99.99"` |
| `as=(object)`| (initial)| `<empty string>` |
| `as=(numeric string)`| (initial)| `<empty string>` |
| `as=(-1)`| (initial)| `<empty string>` |
| `as=(0)`| (initial)| `<empty string>` |
| `as=(integer)`| (initial)| `<empty string>` |
| `as=(NaN)`| (initial, warning)| `<empty string>` |
| `as=(float)`| (initial)| `<empty string>` |
| `as=(true)`| (initial, warning)| `<empty string>` |
| `as=(false)`| (initial, warning)| `<empty string>` |
| `as=(string 'true')`| (changed)| `"true"` |
| `as=(string 'false')`| (changed)| `"false"` |
| `as=(string 'on')`| (changed)| `"on"` |
| `as=(string 'off')`| (changed)| `"off"` |
| `as=(string 'true')`| (initial)| `<empty string>` |
| `as=(string 'false')`| (initial)| `<empty string>` |
| `as=(string 'on')`| (initial)| `<empty string>` |
| `as=(string 'off')`| (initial)| `<empty string>` |
| `as=(symbol)`| (initial, warning)| `<empty string>` |
| `as=(function)`| (initial, warning)| `<empty string>` |
| `as=(null)`| (initial)| `<empty string>` |
Expand Down Expand Up @@ -5251,52 +5251,52 @@
## `initialChecked` (on `<div>`)
| Test Case | Flags | Result |
| --- | --- | --- |
| `initialChecked=(string)`| (changed)| `"a string"` |
| `initialChecked=(empty string)`| (changed)| `<empty string>` |
| `initialChecked=(array with string)`| (changed)| `"string"` |
| `initialChecked=(empty array)`| (changed)| `<empty string>` |
| `initialChecked=(object)`| (changed)| `"result of toString()"` |
| `initialChecked=(numeric string)`| (changed)| `"42"` |
| `initialChecked=(-1)`| (changed)| `"-1"` |
| `initialChecked=(0)`| (changed)| `"0"` |
| `initialChecked=(integer)`| (changed)| `"1"` |
| `initialChecked=(string)`| (changed, warning)| `"a string"` |
| `initialChecked=(empty string)`| (changed, warning)| `<empty string>` |
| `initialChecked=(array with string)`| (changed, warning)| `"string"` |
| `initialChecked=(empty array)`| (changed, warning)| `<empty string>` |
| `initialChecked=(object)`| (changed, warning)| `"result of toString()"` |
| `initialChecked=(numeric string)`| (changed, warning)| `"42"` |
| `initialChecked=(-1)`| (changed, warning)| `"-1"` |
| `initialChecked=(0)`| (changed, warning)| `"0"` |
| `initialChecked=(integer)`| (changed, warning)| `"1"` |
| `initialChecked=(NaN)`| (changed, warning)| `"NaN"` |
| `initialChecked=(float)`| (changed)| `"99.99"` |
| `initialChecked=(float)`| (changed, warning)| `"99.99"` |
| `initialChecked=(true)`| (initial, warning)| `<null>` |
| `initialChecked=(false)`| (initial, warning)| `<null>` |
| `initialChecked=(string 'true')`| (changed)| `"true"` |
| `initialChecked=(string 'false')`| (changed)| `"false"` |
| `initialChecked=(string 'on')`| (changed)| `"on"` |
| `initialChecked=(string 'off')`| (changed)| `"off"` |
| `initialChecked=(string 'true')`| (changed, warning)| `"true"` |
| `initialChecked=(string 'false')`| (changed, warning)| `"false"` |
| `initialChecked=(string 'on')`| (changed, warning)| `"on"` |
| `initialChecked=(string 'off')`| (changed, warning)| `"off"` |
| `initialChecked=(symbol)`| (initial, warning)| `<null>` |
| `initialChecked=(function)`| (initial, warning)| `<null>` |
| `initialChecked=(null)`| (initial)| `<null>` |
| `initialChecked=(undefined)`| (initial)| `<null>` |
| `initialChecked=(null)`| (initial, warning)| `<null>` |
| `initialChecked=(undefined)`| (initial, warning)| `<null>` |

## `initialValue` (on `<div>`)
| Test Case | Flags | Result |
| --- | --- | --- |
| `initialValue=(string)`| (changed)| `"a string"` |
| `initialValue=(empty string)`| (changed)| `<empty string>` |
| `initialValue=(array with string)`| (changed)| `"string"` |
| `initialValue=(empty array)`| (changed)| `<empty string>` |
| `initialValue=(object)`| (changed)| `"result of toString()"` |
| `initialValue=(numeric string)`| (changed)| `"42"` |
| `initialValue=(-1)`| (changed)| `"-1"` |
| `initialValue=(0)`| (changed)| `"0"` |
| `initialValue=(integer)`| (changed)| `"1"` |
| `initialValue=(string)`| (changed, warning)| `"a string"` |
| `initialValue=(empty string)`| (changed, warning)| `<empty string>` |
| `initialValue=(array with string)`| (changed, warning)| `"string"` |
| `initialValue=(empty array)`| (changed, warning)| `<empty string>` |
| `initialValue=(object)`| (changed, warning)| `"result of toString()"` |
| `initialValue=(numeric string)`| (changed, warning)| `"42"` |
| `initialValue=(-1)`| (changed, warning)| `"-1"` |
| `initialValue=(0)`| (changed, warning)| `"0"` |
| `initialValue=(integer)`| (changed, warning)| `"1"` |
| `initialValue=(NaN)`| (changed, warning)| `"NaN"` |
| `initialValue=(float)`| (changed)| `"99.99"` |
| `initialValue=(float)`| (changed, warning)| `"99.99"` |
| `initialValue=(true)`| (initial, warning)| `<null>` |
| `initialValue=(false)`| (initial, warning)| `<null>` |
| `initialValue=(string 'true')`| (changed)| `"true"` |
| `initialValue=(string 'false')`| (changed)| `"false"` |
| `initialValue=(string 'on')`| (changed)| `"on"` |
| `initialValue=(string 'off')`| (changed)| `"off"` |
| `initialValue=(string 'true')`| (changed, warning)| `"true"` |
| `initialValue=(string 'false')`| (changed, warning)| `"false"` |
| `initialValue=(string 'on')`| (changed, warning)| `"on"` |
| `initialValue=(string 'off')`| (changed, warning)| `"off"` |
| `initialValue=(symbol)`| (initial, warning)| `<null>` |
| `initialValue=(function)`| (initial, warning)| `<null>` |
| `initialValue=(null)`| (initial)| `<null>` |
| `initialValue=(undefined)`| (initial)| `<null>` |
| `initialValue=(null)`| (initial, warning)| `<null>` |
| `initialValue=(undefined)`| (initial, warning)| `<null>` |

## `inlist` (on `<div>`)
| Test Case | Flags | Result |
Expand Down Expand Up @@ -9276,27 +9276,27 @@
## `selectedIndex` (on `<div>`)
| Test Case | Flags | Result |
| --- | --- | --- |
| `selectedIndex=(string)`| (initial)| `<number: -1>` |
| `selectedIndex=(empty string)`| (initial)| `<number: -1>` |
| `selectedIndex=(array with string)`| (initial)| `<number: -1>` |
| `selectedIndex=(empty array)`| (initial)| `<number: -1>` |
| `selectedIndex=(object)`| (initial)| `<number: -1>` |
| `selectedIndex=(numeric string)`| (initial)| `<number: -1>` |
| `selectedIndex=(-1)`| (initial)| `<number: -1>` |
| `selectedIndex=(0)`| (initial)| `<number: -1>` |
| `selectedIndex=(integer)`| (initial)| `<number: -1>` |
| `selectedIndex=(string)`| (initial, warning)| `<number: -1>` |
Copy link
Collaborator

Choose a reason for hiding this comment

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

I love that we finally got a nice warning on these for free.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Although I guess this will just make people try selectedindex instead. 😕

| `selectedIndex=(empty string)`| (initial, warning)| `<number: -1>` |
| `selectedIndex=(array with string)`| (initial, warning)| `<number: -1>` |
| `selectedIndex=(empty array)`| (initial, warning)| `<number: -1>` |
| `selectedIndex=(object)`| (initial, warning)| `<number: -1>` |
| `selectedIndex=(numeric string)`| (initial, warning)| `<number: -1>` |
| `selectedIndex=(-1)`| (initial, warning)| `<number: -1>` |
| `selectedIndex=(0)`| (initial, warning)| `<number: -1>` |
| `selectedIndex=(integer)`| (initial, warning)| `<number: -1>` |
| `selectedIndex=(NaN)`| (initial, warning)| `<number: -1>` |
| `selectedIndex=(float)`| (initial)| `<number: -1>` |
| `selectedIndex=(float)`| (initial, warning)| `<number: -1>` |
| `selectedIndex=(true)`| (initial, warning)| `<number: -1>` |
| `selectedIndex=(false)`| (initial, warning)| `<number: -1>` |
| `selectedIndex=(string 'true')`| (initial)| `<number: -1>` |
| `selectedIndex=(string 'false')`| (initial)| `<number: -1>` |
| `selectedIndex=(string 'on')`| (initial)| `<number: -1>` |
| `selectedIndex=(string 'off')`| (initial)| `<number: -1>` |
| `selectedIndex=(string 'true')`| (initial, warning)| `<number: -1>` |
| `selectedIndex=(string 'false')`| (initial, warning)| `<number: -1>` |
| `selectedIndex=(string 'on')`| (initial, warning)| `<number: -1>` |
| `selectedIndex=(string 'off')`| (initial, warning)| `<number: -1>` |
| `selectedIndex=(symbol)`| (initial, warning)| `<number: -1>` |
| `selectedIndex=(function)`| (initial, warning)| `<number: -1>` |
| `selectedIndex=(null)`| (initial)| `<number: -1>` |
| `selectedIndex=(undefined)`| (initial)| `<number: -1>` |
| `selectedIndex=(null)`| (initial, warning)| `<number: -1>` |
| `selectedIndex=(undefined)`| (initial, warning)| `<number: -1>` |

## `shape` (on `<div>`)
| Test Case | Flags | Result |
Expand Down
22 changes: 21 additions & 1 deletion src/renderers/dom/shared/__tests__/ReactDOMAttribute-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ describe('ReactDOM unknown attribute', () => {
expectDev(console.error.calls.count()).toBe(1);
});

it('coerces objects to strings **and warns**', () => {
it('coerces objects to strings and warns', () => {
const lol = {
toString() {
return 'lol';
Expand Down Expand Up @@ -133,5 +133,25 @@ describe('ReactDOM unknown attribute', () => {
);
expectDev(console.error.calls.count()).toBe(1);
});

it('allows camelCase unknown attributes and warns', () => {
spyOn(console, 'error');

var el = document.createElement('div');
ReactDOM.render(<div helloWorld="something" />, el);
expect(el.firstChild.getAttribute('helloworld')).toBe('something');

expectDev(console.error.calls.count()).toBe(1);
expectDev(
normalizeCodeLocInfo(console.error.calls.argsFor(0)[0]),
).toMatch(
'React does not recognize the `helloWorld` prop on a DOM element. ' +
'If you intentionally want it to appear in the DOM as a custom ' +
'attribute, spell it as lowercase `helloworld` instead. ' +
'If you accidentally passed it from a parent component, remove ' +
'it from the DOM element.\n' +
' in div (at **)',
);
});
});
});
38 changes: 32 additions & 6 deletions src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2125,13 +2125,41 @@ describe('ReactDOMComponent', () => {
});

it('allows cased data attributes', function() {
spyOn(console, 'error');

var el = ReactTestUtils.renderIntoDocument(<div data-fooBar="true" />);
expect(el.getAttribute('data-foobar')).toBe('true');

expectDev(console.error.calls.count()).toBe(1);
expectDev(
normalizeCodeLocInfo(console.error.calls.argsFor(0)[0]),
).toMatch(
'React does not recognize the `data-fooBar` prop on a DOM element. ' +
'If you intentionally want it to appear in the DOM as a custom ' +
'attribute, spell it as lowercase `data-foobar` instead. ' +
'If you accidentally passed it from a parent component, remove ' +
'it from the DOM element.\n' +
' in div (at **)',
);
});

it('allows cased custom attributes', function() {
spyOn(console, 'error');

var el = ReactTestUtils.renderIntoDocument(<div fooBar="true" />);
expect(el.getAttribute('foobar')).toBe('true');

expectDev(console.error.calls.count()).toBe(1);
expectDev(
normalizeCodeLocInfo(console.error.calls.argsFor(0)[0]),
).toMatch(
'React does not recognize the `fooBar` prop on a DOM element. ' +
'If you intentionally want it to appear in the DOM as a custom ' +
'attribute, spell it as lowercase `foobar` instead. ' +
'If you accidentally passed it from a parent component, remove ' +
'it from the DOM element.\n' +
' in div (at **)',
);
});

it('warns on NaN attributes', function() {
Expand Down Expand Up @@ -2195,10 +2223,8 @@ describe('ReactDOMComponent', () => {
ReactDOM.render(<svg arabicForm={obj} />, container);
expect(container.firstChild.getAttribute('arabic-form')).toBe('hello');

ReactDOM.render(<div customAttribute={obj} />, container);
expect(container.firstChild.getAttribute('customAttribute')).toBe(
'hello',
);
ReactDOM.render(<div unknown={obj} />, container);
expect(container.firstChild.getAttribute('unknown')).toBe('hello');
});

it('passes objects on known SVG attributes if they do not define toString', () => {
Expand All @@ -2215,8 +2241,8 @@ describe('ReactDOMComponent', () => {
var obj = {};
var container = document.createElement('div');

ReactDOM.render(<div customAttribute={obj} />, container);
expect(container.firstChild.getAttribute('customAttribute')).toBe(
ReactDOM.render(<div unknown={obj} />, container);
expect(container.firstChild.getAttribute('unknown')).toBe(
'[object Object]',
);
});
Expand Down
20 changes: 10 additions & 10 deletions src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -909,25 +909,25 @@ describe('ReactDOMServerIntegration', () => {
});

itRenders('unknown data- attributes with casing', async render => {
const e = await render(<div data-fooBar="true" />);
expect(e.getAttribute('data-fooBar')).toBe('true');
const e = await render(<div data-fooBar="true" />, 1);
expect(e.getAttribute('data-foobar')).toBe('true');
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changing these calls wasn't essential but I figured I'd emphasize they are normalized by the browser anyway.

});

itRenders('unknown data- attributes with boolean true', async render => {
const e = await render(<div data-fooBar={true} />);
expect(e.getAttribute('data-fooBar')).toBe('true');
const e = await render(<div data-foobar={true} />);
expect(e.getAttribute('data-foobar')).toBe('true');
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 tests were not specifically about casing so I fixed them.

});

itRenders('unknown data- attributes with boolean false', async render => {
const e = await render(<div data-fooBar={false} />);
expect(e.getAttribute('data-fooBar')).toBe('false');
const e = await render(<div data-foobar={false} />);
expect(e.getAttribute('data-foobar')).toBe('false');
});

itRenders(
'no unknown data- attributes with casing and null value',
async render => {
const e = await render(<div data-fooBar={null} />);
expect(e.hasAttribute('data-fooBar')).toBe(false);
const e = await render(<div data-fooBar={null} />, 1);
expect(e.hasAttribute('data-foobar')).toBe(false);
},
);

Expand Down Expand Up @@ -972,8 +972,8 @@ describe('ReactDOMServerIntegration', () => {
});

itRenders('cased custom attributes', async render => {
const e = await render(<div fooBar="test" />);
expect(e.getAttribute('fooBar')).toBe('test');
const e = await render(<div fooBar="test" />, 1);
expect(e.getAttribute('foobar')).toBe('test');
});
});

Expand Down
27 changes: 24 additions & 3 deletions src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,10 @@ if (__DEV__) {
var warnedProperties = {};
var hasOwnProperty = Object.prototype.hasOwnProperty;
var EVENT_NAME_REGEX = /^on[A-Z]/;
var ARIA_NAME_REGEX = /^aria-/i;
var rARIA = new RegExp('^(aria)-[' + DOMProperty.ATTRIBUTE_NAME_CHAR + ']*$');
var rARIACamel = new RegExp(
'^(aria)[A-Z][' + DOMProperty.ATTRIBUTE_NAME_CHAR + ']*$',
);
Copy link
Collaborator Author

@gaearon gaearon Sep 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Copy pasta from ARIA hook. Catches more cases which ensures we don't fire two warnings instead of one.

var possibleStandardNames = require('possibleStandardNames');

var validateProperty = function(tagName, name, value, debugID) {
Expand Down Expand Up @@ -91,7 +94,7 @@ if (__DEV__) {
}

// Let the ARIA attribute hook validate ARIA attributes
if (ARIA_NAME_REGEX.test(name)) {
if (rARIA.test(name) || rARIACamel.test(name)) {
return true;
}

Expand Down Expand Up @@ -155,6 +158,8 @@ if (__DEV__) {
return true;
}

const isReserved = DOMProperty.isReservedProp(name);

// Known attributes should match the casing specified in the property config.
if (possibleStandardNames.hasOwnProperty(lowerCasedName)) {
var standardName = possibleStandardNames[lowerCasedName];
Expand All @@ -169,6 +174,22 @@ if (__DEV__) {
warnedProperties[name] = true;
return true;
}
} else if (!isReserved && name !== lowerCasedName) {
// Unknown attributes should have lowercase casing since that's how they
// will be cased anyway with server rendering.
warning(
false,
'React does not recognize the `%s` prop on a DOM element. If you ' +
'intentionally want it to appear in the DOM as a custom ' +
'attribute, spell it as lowercase `%s` instead. ' +
'If you accidentally passed it from a parent component, remove ' +
'it from the DOM element.%s',
name,
lowerCasedName,
getStackAddendum(debugID),
);
warnedProperties[name] = true;
return true;
}

if (typeof value === 'boolean') {
Expand All @@ -186,7 +207,7 @@ if (__DEV__) {

// Now that we've validated casing, do not validate
// data types for reserved props
if (DOMProperty.isReservedProp(name)) {
if (isReserved) {
return true;
}

Expand Down