[Documentation] Re-add "Transferring with ... in JSX" to "JSX In-Depth / Spread Attributes"

[hellobrian]

resolves #109

Re-add Transferring with ... in JSX docs previously published with React v15-ish.

The overall content has been kept the same but I slightly changed the example to better reflect why using ...other can be useful (I also wanted to avoid an example that uses a <div> as a checkbox).

In the example, I show a PrimaryButton component that can receive an unknown onClick prop function. Things like buttons handle a lot of different events -- instead of passing every possible synthetic event down as a prop, making use of ...other helps to keep the code for PrimaryButton from getting bloated.

If it helps, I've also made the example live on code sandbox but didn't include it...I wasn't sure if it was totally needed, and since the docs surface codepen examples exclusively. I don't know if you all want to stay consistent with one example platform.

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[reactjs-bot]

Deploy preview ready!

Built with commit 39ef13f

https://deploy-preview-121--reactjs.netlify.com

[bvaughn]

I slightly prefer the original presentation. It quickly explained what spread props were before going into why they might be problematic. Thoughts?

[bvaughn]

This line number (7) no longer points to the thing I think you want to highlight. I believe you want line 6 (where the other props are actually passed to <button>)>

[bvaughn]

Could we pick a different example? Since in this case type would be passed the same regardless of whether you plucked it out of props or passed it via other.

[hellobrian]

I didn't know what this markdown syntax was before, changed it to highlight line 6 like you suggested.

[hellobrian]

Could we pick a different example? Since in this case type would be passed the same regardless of whether you plucked it out of props or passed it via other. - @bvaughn

To your original comment, I updated the example removing type from this line. I wasn't trying to focus on type but I was trying to show how PrimaryButton could now receive unknown props like onClick

<PrimaryButton type="button" onClick={handleClick}>

If this still isn't preferrable, I can just use the original example as-is from the original docs (Transferring with ... in JSX)

[bvaughn]

I think this is better! But it's still a little awkward since you could also pass children via the spread operator as well- and <button type="button"> isn't a valid attribute. These might be confusing to readers. (They might get distracted thinking, "why is that..." instead of focusing on the spread operator.)

How do you feel about something like this?

const handleClick = () => console.log("handleClick");

const PrimaryButton = props => {
  const { type, ...other } = props;
  const className = type === "primary" ? "PrimaryButton" : "SecondaryButton";
  return <button className={className} {...other} />;
};

const App = () => {
  return (
    <div>
      <PrimaryButton type="primary" onClick={handleClick}>
        Hello World!
      </PrimaryButton>
    </div>
  );
};

[hellobrian]

This works too, but <button type="button"> is a valid attribute (see MDN). Even though the validity of the attribute is important, I agree, it could be confusing for readers either way. Instead, we can strip down this example even further so that it shows all the props coming from ...other

const handleClick = () => console.log("handleClick");

const PrimaryButton = props => {
  const { ...other } = props;
  return <button {...other} />;
};

const App = () => {
  return (
    <div>
      <PrimaryButton onClick={handleClick}>
        Hello World!
      </PrimaryButton>
    </div>
  );
};

☝️ How about this? Also, thanks for taking a look at this!

[bvaughn]

<button type="button"> is a valid attribute

You are correct, of course. Sometimes I forget how wonky HTML attributes are 😆

const { ...other } = props;
return <button {...other} />;

I think this maybe leaves too much to the imagination. "Why would I do this? What's the point?" I think we should show at least one prop being plucked out.

[hellobrian]

😆Alright, with everything said so far,

I can pluck className instead of type to avoid confusion
(hopefully, I'm using the "pluck" terminology correctly)

The PrimaryButton component takes a className prop. It also takes any number of unknown props through the ...other object, just as long as they are valid HTML attributes for <button>.

const handleClick = () => console.log("handleClick");

const PrimaryButton = props => {
  const { className, ...other } = props;
  return <button className={className} {...other} />;
};

const App = () => {
  return (
    <div>
      <PrimaryButton className="primaryButton" onClick={handleClick}>
        Hello World!
      </PrimaryButton>
    </div>
  );
};

Now, the App component can return the PrimaryButton so that it can receive it's known props (className) and any unknown props through ...other, which contains the following when used in App:

// Contained in { ...other }
{ 
  onClick: handleClick(), 
  children: 'Hello World!' 
}

[bvaughn]

I would have the same concern as I mentioned before:

// This...
const PrimaryButton = props => {
  const { className, ...other } = props;
  return <button className={className} {...other} />;
};

// Is the same thing as this...
const PrimaryButton = props => {
  return <button {...props} />;
};

I think we should pick a prop out that we don't want to pass on to reinforce the idea of why you might want to pluck things out. It's true that you will sometimes filter a prop out and still pass it through but I think that's not the main point of demonstrating the technique in this section of the docs.

[hellobrian]

In response to: #121 (comment) (comment thread was getting long)

Thanks for your patience, I think I finally understand what you've been trying to say 😭
My motivation for re-adding this documentation was to surface how you can pass unknown props via spread attributes. But, I can see now that there are more considerations that need to be made when opting to use this syntax, which is what you were trying to communicate.

Thanks again for being so thorough with this PR, @bvaughn

[bvaughn]

No problem at all 😄 It's easy to miscommunicate on a PR.

[bvaughn]

nit: I think this sentence may not be necessary. I think it might be nice to just show the example and then explain it below (like you already do) 👍

[bvaughn]

Suggestion / nit:

In the example above, the kind prop is safely consumed and is not passed on to the <button> element in the DOM.

[bvaughn]

Suggestion:

Spread attributes can be useful but they also make it easy to pass unnecessary props to components that don't care about them or to pass invalid HTML attributes to the DOM. We recommend using this syntax sparingly.

[bvaughn]

Thanks for iterating on this with me 👍