5 pointsby todsacerdoti9 days ago2 comments
  • Noumenon729 days ago
    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.
  • 9 days ago
    undefined