I thought the example comment should strive to be shorter than this:
// Currently, key can be spread in as a prop. This causes a potential
// issue if key is also explicitly declared (ie. <div {...props} key="Hi" />
// or <div key="Hi" {...props} /> ). We want to deprecate key spread,
// but as an intermediary step, we will use jsxDEV for everything except
// <div {...props} key="Hi" />, because we aren't currently able to tell if
// key is explicitly declared to be undefined or not.
if (maybeKey !== undefined) {
key = '' + maybeKey
}
But I found while trying to shorten it that a) it couldn't be that much shorter and b) it actually contained all the info I needed to understand it, which is the goal. Rewriting to my preference would just be my way of understanding it.
Anyway, here's what I would write to explain the why:
// If the JSX key was passed explicitly, use it. This may overwrite a `key`
// from spread props, e.g. <div {...props} key="Hi" />. We can't distinguish
// that from <div key="Hi" {...props} />, so until key spreading is deprecated,
// we ask people to use `jsxDEV` instead of `jsx` for all cases except
// <div {...props} key="Hi" />.
This explains:
* Why the line of code is here: to ensure explicitly passed `key` wins. The 'what' is "coerce maybeKey to a string", which doesn't clearly address the issue of key spread
* State the issue with _this line of code_, which is that it overwrites user-specified keys in props, not just that it's "a potential issue".
* State which example is good and which is bad and requires jsxDEV -- actionable advice.
I also think the `key` param should be documented to say
This will overwrite `key` in spread props. Use `jsxDEV` if you want to do <div key="Hi" {...props} />.
Otherwise, you've only explained the issue to internal developers, while framework users get no warning until they're so far into debugging they've narrowed it down to this function.
I thought the example comment should strive to be shorter than this:
But I found while trying to shorten it that a) it couldn't be that much shorter and b) it actually contained all the info I needed to understand it, which is the goal. Rewriting to my preference would just be my way of understanding it.Anyway, here's what I would write to explain the why:
This explains:* Why the line of code is here: to ensure explicitly passed `key` wins. The 'what' is "coerce maybeKey to a string", which doesn't clearly address the issue of key spread
* State the issue with _this line of code_, which is that it overwrites user-specified keys in props, not just that it's "a potential issue".
* State which example is good and which is bad and requires jsxDEV -- actionable advice.
I also think the `key` param should be documented to say
Otherwise, you've only explained the issue to internal developers, while framework users get no warning until they're so far into debugging they've narrowed it down to this function.