Skip to content

Warn for keys in fragments - second approach #9434

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
flarnie wants to merge 6 commits into from
Closed

Warn for keys in fragments - second approach #9434

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
23252d5
Pull out `validateExplicitKey` from `ReactElementValidator`
flarnie Apr 14, 2017
d6cfec8
Fix tests to pass when we warn for missing keys in fragments
flarnie Apr 13, 2017
c4d28a0
Warn for missing keys in fragments returned by React components
flarnie Apr 14, 2017
398b0fb
Commit updates to scripts/rollup/results.json
flarnie Apr 14, 2017
c84c0d9
Ran 'prettier' to remove syntax lints
flarnie Apr 14, 2017
adf9153
Refactor further to allow 'warning' to get hard coded string
flarnie Apr 16, 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
50 changes: 25 additions & 25 deletions scripts/rollup/results.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
{
"branch": "master",
"branch": "warnForKeysInFragmentsRefactor",
Copy link
Copy Markdown
Contributor

@aweary aweary Apr 14, 2017

Choose a reason for hiding this comment

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

What's the policy for committing changes to results.json? It would be weird if branch was always just the name of the last branch merged into master.

Copy link
Copy Markdown
Collaborator

@gaearon gaearon Apr 14, 2017

Choose a reason for hiding this comment

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

That's what we do for now. It doesn't really matter and gives us heads up on unexpected changes. Ideally later we'd migrate to another approach (e.g. a bot that comments). But this works okay for now.

Copy link
Copy Markdown
Contributor

@aweary aweary Apr 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

So should we be asking contributors to include this in PRs that make changes to code included in bundles? If so I can add it to our PR template for now.

Copy link
Copy Markdown
Collaborator

@sophiebits sophiebits Apr 14, 2017

Choose a reason for hiding this comment

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

Won't we get terrible merge conflicts soon once everyone has a different base here? :)

Copy link
Copy Markdown
Contributor Author

@flarnie flarnie Apr 16, 2017

Choose a reason for hiding this comment

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

tldr; In most cases no, there won't be terrible merge conflicts, and when it happens it should be fairly easy to resolve.

I think I have been in similar situations with generated 'schema' type files, and there can be annoying merge conflicts in some cases.

If people have a stack of commits, and they commit the 'results.json' in each commit, and then rebase, they could have a merge conflict to resolve for each diff.

Ideally they just commit the 'results.json' at the top of their commit stack, and if there is a conflict they can regenerate the file and git add scripts/rollup/results.json.

It would be great to automate this though.

"bundleSizes": {
"react.development.js (UMD_DEV)": {
"size": 121454,
"gzip": 30515
"size": 122649,
"gzip": 30785
},
"react.production.min.js (UMD_PROD)": {
"size": 15685,
"gzip": 5765
},
"react-dom.development.js (UMD_DEV)": {
"size": 583190,
"gzip": 134534
"size": 585217,
"gzip": 135179
},
"react-dom.production.min.js (UMD_PROD)": {
"size": 120740,
Expand All @@ -26,24 +26,24 @@
"gzip": 33273
},
"react-art.development.js (UMD_DEV)": {
"size": 342608,
"gzip": 76782
"size": 344635,
"gzip": 77415
},
"react-art.production.min.js (UMD_PROD)": {
"size": 95013,
"gzip": 28991
},
"react.development.js (NODE_DEV)": {
"size": 70266,
"gzip": 17594
"size": 71459,
"gzip": 17851
},
"react.production.min.js (NODE_PROD)": {
"size": 9226,
"gzip": 3628
},
"React-dev.js (FB_DEV)": {
"size": 72123,
"gzip": 18231
"size": 73317,
"gzip": 18490
},
"React-prod.js (FB_PROD)": {
"size": 36643,
Expand All @@ -58,16 +58,16 @@
"gzip": 84675
},
"react-dom.development.js (NODE_DEV)": {
"size": 542188,
"gzip": 125158
"size": 544213,
"gzip": 125804
},
"react-dom.production.min.js (NODE_PROD)": {
"size": 116925,
"gzip": 36732
},
"ReactDOMFiber-dev.js (FB_DEV)": {
"size": 797235,
"gzip": 184122
"size": 799262,
"gzip": 184749
},
"ReactDOMFiber-prod.js (FB_PROD)": {
"size": 407613,
Expand Down Expand Up @@ -98,16 +98,16 @@
"gzip": 22993
},
"react-art.development.js (NODE_DEV)": {
"size": 265052,
"gzip": 56927
"size": 267077,
"gzip": 57558
},
"react-art.production.min.js (NODE_PROD)": {
"size": 56628,
"gzip": 17152
},
"ReactARTFiber-dev.js (FB_DEV)": {
"size": 264230,
"gzip": 56736
"size": 266255,
"gzip": 57354
},
"ReactARTFiber-prod.js (FB_PROD)": {
"size": 205336,
Expand All @@ -122,20 +122,20 @@
"gzip": 84001
},
"ReactTestRendererFiber-dev.js (FB_DEV)": {
"size": 262139,
"gzip": 55704
"size": 264164,
"gzip": 56321
},
"ReactTestRendererStack-dev.js (FB_DEV)": {
"size": 151521,
"gzip": 34765
},
"react-noop-renderer.development.js (NODE_DEV)": {
"size": 254136,
"gzip": 53682
"size": 256161,
"gzip": 54312
},
"react-test-renderer.development.js (NODE_DEV)": {
"size": 262970,
"gzip": 55891
"size": 264995,
"gzip": 56524
}
}
}
46 changes: 46 additions & 0 deletions src/isomorphic/children/__tests__/ReactChildren-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,17 @@

'use strict';

const ReactDOMFeatureFlags = require('ReactDOMFeatureFlags');

describe('ReactChildren', () => {
var React;
var ReactTestUtils;
var ReactFeatureFlags;

beforeEach(() => {
jest.resetModules();
React = require('react');
ReactTestUtils = require('ReactTestUtils');
});

it('should support identity for simple', () => {
Expand Down Expand Up @@ -850,4 +855,45 @@ describe('ReactChildren', () => {
'to render a collection of children, use an array instead.',
);
});

if (ReactDOMFeatureFlags.useFiber) {
describe('with fragments enabled', () => {
beforeEach(() => {
ReactFeatureFlags = require('ReactFeatureFlags');
ReactFeatureFlags.disableNewFiberFeatures = false;
});

it('warns for keys for arrays of elements in a fragment', () => {
spyOn(console, 'error');
class ComponentReturningArray extends React.Component {
render() {
return [<div />, <div />];
}
}

ReactTestUtils.renderIntoDocument(
React.createElement(ComponentReturningArray),
);

expectDev(console.error.calls.count()).toBe(1);
expectDev(console.error.calls.argsFor(0)[0]).toContain(
'Warning: ' +
'Each child in an array or iterator should have a unique "key" prop.',
);
});

it('does not warn when there are keys on elements in a fragment', () => {
spyOn(console, 'error');
class ComponentReturningArray extends React.Component {
render() {
return [<div key="foo" />, <div key="bar" />];
}
}

ReactTestUtils.renderIntoDocument(<ComponentReturningArray />);

expectDev(console.error.calls.count()).toBe(0);
});
});
}
});
66 changes: 30 additions & 36 deletions src/isomorphic/classic/element/ReactElementValidator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ var ReactElement = require('ReactElement');
var canDefineProperty = require('canDefineProperty');
var getComponentName = require('getComponentName');
var getIteratorFn = require('getIteratorFn');
var validateExplicitKey = require('validateExplicitKey');

if (__DEV__) {
var checkPropTypes = require('checkPropTypes');
Expand Down Expand Up @@ -58,13 +59,6 @@ function getSourceInfoErrorAddendum(elementProps) {
return '';
}

/**
* Warn if there's no key explicitly set on dynamic arrays of children or
* object keys are not valid. This allows us to keep track of children between
* updates.
*/
var ownerHasKeyUseWarning = {};

function getCurrentComponentErrorInfo(parentType) {
var info = getDeclarationErrorAddendum();

Expand All @@ -80,30 +74,16 @@ function getCurrentComponentErrorInfo(parentType) {
}

/**
* Warn if the element doesn't have an explicit key assigned to it.
* This element is in an array. The array could grow and shrink or be
* reordered. All children that haven't already been validated are required to
* have a "key" property assigned to it. Error statuses are cached so a warning
* will only be shown once.
* Composes the warning that is shown when an element doesn't have an explicit
* key assigned to it.
*
* @internal
* @param {ReactElement} element Element that requires a key.
* @param {*} parentType element's parent's type.
* @param {ReactElement} element Element that requires a key.
* @return string message that described explicit key error
*/
function validateExplicitKey(element, parentType) {
if (!element._store || element._store.validated || element.key != null) {
return;
}
element._store.validated = true;

var memoizer = ownerHasKeyUseWarning.uniqueKey ||
(ownerHasKeyUseWarning.uniqueKey = {});

function getExplicitKeyErrorMessage(parentType, element) {
var currentComponentErrorInfo = getCurrentComponentErrorInfo(parentType);
if (memoizer[currentComponentErrorInfo]) {
return;
}
memoizer[currentComponentErrorInfo] = true;

// Usually the current owner is the offender, but if it accepts children as a
// property, it may be the creator of the child that's responsible for
Expand All @@ -116,14 +96,12 @@ function validateExplicitKey(element, parentType) {
childOwner = ` It was passed a child from ${getComponentName(element._owner)}.`;
}

warning(
false,
'Each child in an array or iterator should have a unique "key" prop.' +
'%s%s See https://fb.me/react-warning-keys for more information.%s',
currentComponentErrorInfo,
childOwner,
getCurrentStackAddendum(element),
);
const message = 'Each child in an array or iterator should have a unique "key" prop.' +
currentComponentErrorInfo +
childOwner +
' ' +
'See https://fb.me/react-warning-keys for more information.';
return message;
}

/**
Expand All @@ -143,7 +121,15 @@ function validateChildKeys(node, parentType) {
for (var i = 0; i < node.length; i++) {
var child = node[i];
if (ReactElement.isValidElement(child)) {
validateExplicitKey(child, parentType);
const getMainExplicitKeyErrorMessage = getExplicitKeyErrorMessage.bind(
null,
parentType,
);
Copy link
Copy Markdown
Contributor

@aweary aweary Apr 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I wonder if there's a way we could do this without having to create a new function for every child? What if parentType was passed to validateExplicitKey and it was called from there instead?

If not, we should move this outside of the for loop so the function is created once per call.

Copy link
Copy Markdown
Contributor Author

@flarnie flarnie Apr 16, 2017

Choose a reason for hiding this comment

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

Nice catch - will fix.

Copy link
Copy Markdown
Contributor Author

@flarnie flarnie Apr 16, 2017

Choose a reason for hiding this comment

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

I think the cleanest option is to just move it outside of the 'for' loop. I'll play around with some alternatives and maybe push another version to see, but I think it will be ugly.

validateExplicitKey(
child,
getMainExplicitKeyErrorMessage,
getCurrentStackAddendum,
);
}
}
} else if (ReactElement.isValidElement(node)) {
Expand All @@ -160,7 +146,15 @@ function validateChildKeys(node, parentType) {
var step;
while (!(step = iterator.next()).done) {
if (ReactElement.isValidElement(step.value)) {
validateExplicitKey(step.value, parentType);
const getMainExplicitKeyErrorMessage = getExplicitKeyErrorMessage.bind(
Copy link
Copy Markdown
Contributor

@aweary aweary Apr 14, 2017

Choose a reason for hiding this comment

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

Same here. If we bind outside of the loop (maybe at the top of validateChildKeys) we can save some work

Copy link
Copy Markdown
Contributor Author

@flarnie flarnie Apr 16, 2017

Choose a reason for hiding this comment

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

Thanks! Will fix.

null,
parentType,
);
validateExplicitKey(
step.value,
getMainExplicitKeyErrorMessage,
getCurrentStackAddendum,
);
}
}
}
Expand Down
18 changes: 14 additions & 4 deletions src/renderers/__tests__/ReactMultiChildText-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,10 +191,20 @@ describe('ReactMultiChildText', () => {
[true, <div>{1.2}{''}{<div />}{'foo'}</div>, true, 1.2], [<div />, '1.2'],
['', 'foo', <div>{true}{<div />}{1.2}{''}</div>, 'foo'], ['', 'foo', <div />, 'foo'],
]);
expectDev(console.error.calls.count()).toBe(1);
expectDev(console.error.calls.argsFor(0)[0]).toContain(
'Warning: Each child in an array or iterator should have a unique "key" prop.',
);
if (ReactDOMFeatureFlags.useFiber) {
expectDev(console.error.calls.count()).toBe(2);
expectDev(console.error.calls.argsFor(0)[0]).toContain(
'Warning: Each child in an array or iterator should have a unique "key" prop.',
);
expectDev(console.error.calls.argsFor(1)[0]).toContain(
'Warning: Each child in an array or iterator should have a unique "key" prop.',
);
} else {
expectDev(console.error.calls.count()).toBe(1);
expectDev(console.error.calls.argsFor(0)[0]).toContain(
'Warning: Each child in an array or iterator should have a unique "key" prop.',
);
}
});

it('should throw if rendering both HTML and children', () => {
Expand Down
Loading