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 1 commit
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
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 wanted it to appear in the DOM as a custom ' +
'attribute, spell it as the lowercase `helloworld` instead. ' +
'If you passed it accidentally from a parent component, remove ' +
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it’s important to mention this since people will often get this warning due to spreading.

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like practically this is going to make the unknown DOM warning permanent (modulo some cases). Granted it's probably not what the user intends it seems a bit heavy handed.

Copy link
Contributor

Choose a reason for hiding this comment

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

or perhaps maybe just that it's going to give the impression that everything is fine when they aren't getting this warning, while foo="[Object object]" still gets passed

Copy link
Contributor

Choose a reason for hiding this comment

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

The react prop name can still have casing, as long as it's passed as lowercase to the HTML element. This doesn't allow spreading of attributes, but you can do it at least.

I think it's important to communicate what this is protecting against. I don't know that this should live in the warning, but I do agree that a developer not familiar with the problem-space could see this as an arbitrary restriction.

We could turn it into a "React's got your back" kind of thing if we effectively communicate it.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about custom elements (or does that not hit here)? Chance of a false positive? If we are trying to get out of folks way about passing what they want to the DOM this feels a bit arbitrary as you say.

Copy link
Contributor

Choose a reason for hiding this comment

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

sigh didn't see the original issue, sorry for the noise :P

'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 wanted it to appear in the DOM as a custom ' +
'attribute, spell it as the lowercase `data-foobar` instead. ' +
'If you passed it accidentally 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 wanted it to appear in the DOM as a custom ' +
'attribute, spell it as the lowercase `foobar` instead. ' +
'If you passed it accidentally 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 wanted it to appear in the DOM as a custom ' +
'attribute, spell it as the lowercase `%s` instead. ' +
'If you passed it accidentally 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