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.

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

ReactDOMServer.renderToStaticMarkup should not cause 'iterator should have a unique "key" prop' warning #7038

Closed
mnpenner opened this issue Jun 14, 2016 · 2 comments

Comments

@mnpenner
Copy link
Contributor

Do you want to request a feature or report a bug?

It's more of a bug -- I don't think this behaviour should occur, but it's not a problem in production.

What is the current behavior?

Warning appears.

Warning: Each child in an array or iterator should have a unique "key" prop. Check the render method of List. See https://fb.me/react-warning-keys for more information.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem via https://jsfiddle.net or similar (template: https://jsfiddle.net/reactjs/69z2wepo/).

Fiddle. I'll paste the code here too, just in case:

<script src="https://facebook.github.io/react/js/jsfiddle-integration-babel.js"></script>
<script type="text/javascript" src="https://cdnjs.cloudflare.com/ajax/libs/react/15.1.0/react-dom-server.min.js"></script>

<div id="container">
    <!-- This element's contents will be replaced with your component. -->
</div>
function List(props) {
    return <ul>
    {props.items.map(x => <li>{x}</li>)}
  </ul>
}

let listItems = [...Array(10).keys()];

document.getElementById('container').innerHTML = ReactDOMServer.renderToStaticMarkup(<List items={listItems}/>);

What is the expected behavior?

No warning.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

Fiddle uses React 15.0.1. Tested in Chrome on Ubuntu. I didn't test previous versions of React, but I suspect it's always been this way.

Notes

If I'm not mistaken, the HTML generated via renderToStaticMarkup cannot be updated by React because the essential DOM attributes are stripped away. This means that keys should not be necessary because React will never need to match up the elements during a re-render. Supplying keys that will never be used just to avoid a warning is tedious. Ergo, warning should be suppressed when JSX is rendered via renderToStaticMarkup.

@mnpenner mnpenner changed the title ReactDOMServer.renderToStaticMarkup should not cause "iterate should have a unique key drop" warning ReactDOMServer.renderToStaticMarkup should not cause 'iterator should have a unique "key" prop' warning Jun 14, 2016
@jimfb
Copy link
Contributor

jimfb commented Jun 15, 2016

Yeah, technically true.

There were some talks a while back about potentially doing #6618, in which case, renderToString and renderToStaticMarkup could become a single function because React wouldn't need any additional data in the DOM. That would make this don't-emit-key-warning difference much harder to justify.

There is also something to be said for remaining consistent. If you are writing components that don't specify a key, then your components can't be shared/used with renderToString nor ReactDOM.render in the future (decreases reusability, increases fragmentation). Maybe it would be too opinionated for us to enforce that, but I could make the argument that maintaining a single cohesive component ecosystem is more important.

Honestly, this would be pretty low priority for us. If you see an easy fix and want to submit a PR, I'd say it has a 50/50 chance (coin toss) of getting merged, probably depending primarily on complexity. We don't want to introduce any global state or do a whole ton of routing to decide if this warning needs to be emitted. Probably your best bet would be to move the warning to a devtool, and have the devtool become aware of what type of render is being performed. Nested renders would need to be considered.

I'm going to close this out because this isn't a clear win for us, and it is almost certainly something that our team would not have the bandwidth to do internally. If someone in the community is passionate about fixing this, we'd take a look at any PRs, but can't make any promises a priori.

@jimfb jimfb closed this as completed Jun 15, 2016
@mnpenner
Copy link
Contributor Author

That's fair enough, I wouldn't rate it as high priority either. Thank you for taking the time to respond @jimfb.

aduth added a commit to WordPress/gutenberg that referenced this issue Aug 10, 2017
Needed to ensure key is assigned into map result to avoid React error. Seems unnecessary since we don't need the diffing reconciliation for generating static markup, but alas.

See: facebook/react#7038
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants