"Composition vs Inheritance" and "Lifting State Up" #7920

gaearon · 2016-10-07T23:48:34Z

This is the last "Basics" guide I planned to write.

(new screenshot below)

suchipi · 2016-10-07T23:55:51Z

docs/docs/composition-vs-inheritance.md

+}
+```
+
+Class components also use composition in React:


This should probably be "Class components can also..." (adding the word 'can')

orlando · 2016-10-07T23:58:11Z

@gaearon adding reviews and requesting changes should be only for the collaborators right?. Seems weird to me that is open to everyone.

gaearon · 2016-10-08T00:00:32Z

¯_(ツ)_/¯ 😄

suchipi · 2016-10-08T00:01:26Z

I went out of my way to just do a single comment but it still counts it as a "review", oh well 😅

samit4me · 2016-10-08T01:28:40Z

With the title Composition vs Inheritance, I was expecting to see the PROS and CONS of each paradigm and why to choose one over the other. But this guide appears to be more of a "recommended approach", which is totally fine, I was just expecting more.

I've never attempted to use inheritance in React (apart from extends Component), so I do not know the pitfalls, but I know some backend devs (Java, C#) that would automatically try inheritance, especially with the Specialization example.

So what I am saying, it might be good to give an example of Inheritance and highlight the pitfalls, what are your thoughts @gaearon ?

Great job, this is very clear and concise. I sincerely commend your efforts towards documentation.

nashio · 2016-10-08T06:31:00Z

docs/docs/composition-vs-inheritance.md

+
+Props and composition give you all the flexibility you need to customize a component's look and behavior in an explicit and safe way. Remember that components may accept arbitrary props, including primitive values, React elements, or functions.
+
+If you want to reuse some common functionality between components that is not related to the UI, we suggest extracting it into a plain JavaScript module, and importing and using that module from the components.


This part might need rewording?, here's an idea: "If you want to reuse non-UI functionality between components, we suggest extracting it into a plain JavaScript module so you can import it, and use it as a module inside a component"

anhuiliujun · 2016-10-08T06:38:47Z

great job

gaearon · 2016-10-08T08:32:09Z

So what I am saying, it might be good to give an example of Inheritance and highlight the pitfalls, what are your thoughts @gaearon ?

I thought about it at first but then I’m worried people are going to see an example of inheritance in React docs without reading the text around it and think “okay it’s fine to use”. (Yea people read docs this way.)

As I stress in the last paragraph, we strongly recommend against using it for React components. I expect that after this doc is out, we might see some issues where people argue the opposite, and this might give us an opportunity to add more examples showing alternative approaches.

gaearon · 2016-10-08T08:46:34Z

Tweaked a few sentences per feedback.

gaearon · 2016-10-08T15:05:05Z

Updated with some Codepen examples.

(outdated)

gaearon · 2016-10-08T18:41:15Z

Added another one:

Lifting State Up

(outdated)

gaelollivier · 2016-10-08T19:10:58Z

Really nice write-up ! Really missed this "Lifting state up" when I first learned React, especially being so used to two-way data binding.

One thing that's disturbing me a bit though is having both celsiusText and fahrenheitText in Calculator state. You're talking about having one single source of truth, so maybe it would be good to also prevent readers from having duplication of information. In this case, wouldn't it make sense to only store one value (let's say celsius) and compute the derived one (fahrenheit) at render time ? I guess the reason you didn't do this is to not add another concept to the article but I think it comes quite naturally with the idea of "single source of truth".

gaearon · 2016-10-08T19:24:03Z

Yea, I should probably do that. I didn't want to at first because I thought I'd need to let each of them be "in error state" separately (imagine entering "lol" in one and "wow" in another). However this is not an issue because I decided to clear invalid derived inputs later anyway.

Let me give that a try.

gaearon · 2016-10-08T19:40:43Z

Storing just celsiusText wouldn't be enough because we wouldn't know which input has bad input if it doesn't parse (e.g. "wow"). But we could store {text, activeScale}.

gaelollivier · 2016-10-08T19:59:45Z

I would have say that error handling is a bit out of scope for this kind of tutorial, not sure it's really needed to grasp the core idea.
If it is, how about keeping the error-handling responsibility inside the TemperatureInput, and calling onChange only when the input is a valid float ? It would do something like:

<input
  value={props.text}
  onChange={e => !isNaN(parseFloat(e.target.value || '0')) && props.onChange(e.target.value)}
/>

(The || 0 is to allow the field to be empty. Must admit it's a bit obscure, that's why I wouldn't bother much with error handling at all for this article)

gaearon · 2016-10-08T20:00:48Z

It's not quite about error handling, but about controlled input becoming "stuck" on un-parseable numbers precisely because it is controlled. We need the value to propagate upwards, or user won't be able to type.

gaelollivier · 2016-10-08T20:29:46Z

Indeed, just realised (had to actually run it to grasp the problem) that's not that trivial and suddenly your first solution makes way more sense 👍

suchipi · 2016-10-08T20:34:25Z

It may be useful to include a link to Controlled Inputs before going through the Lifting State Up example. Someone who has not read that before may be confused if they start changing the example on codepen.

gaearon · 2016-10-08T22:46:44Z

@suchipi Yea, "handling forms" will come before that (it's not written yet).

@gaelduplessix It seems like adding type="number" to inputs work well enough in modern browsers that we should just do that and only store this.state.celsium.

gaearon · 2016-10-09T00:30:54Z

Okay, final versions so far.

(outdated)

samit4me · 2016-10-09T02:08:18Z

docs/docs/lifting-state-up.md

+
+  handleChange(e) {
+    this.setState({
+      celsius: parseFloat(e.target.value, 10)


Small nitpick, but I don't think parseFloat() takes a radix, only parseInt(), so I believe the base10 is ignored. This radix is also passed is in a few other places throughout this code.

Thanks, fixed.

kof · 2016-10-09T10:57:34Z

docs/docs/lifting-state-up.md

+    this.state = {celsius: 0};
+  }
+
+  handleChange(e) {


why not to use

handleChange = (e) => { ... }

To avoid the need for a bound function generation in constructor?

Class properties are not officially in the JS spec and require the use of either:

Babel plugin transform-class-properties OR

Bable preset Stage 2 or lower (e.g. Stage 0).

There is some great information regarding modern JS in hello-world.md

Yeah, we don't want to use experimental proposals in docs except in a few places where we specifically call out their experimental nature

draco · 2016-10-09T11:39:21Z

docs/docs/lifting-state-up.md

+
+Our new requirement is that, in addition to a Celsius input, we provide a Fahrenheit input, and they are kept in sync.
+
+We can start by extracting a `TemperatureInput` component from `Calculator`. We will add a new `scale` prop to it that can either be `"C"` or `"F"`. We will also rename the `celsium` state variable to `temperature` because it may now represent a value in either scale:


Nitpick but there's a typo here celsium instead of celsius, it also happened in a few other places as well.

Thanks, fixed

gaearon · 2016-10-09T16:39:57Z

Addressed more feedback. Also tweaked Lifting State Up to avoid the distracting and irrelevant class -> function transformation.

(outdated)

optimatex · 2016-10-09T17:03:41Z

nice articles. Thanks for great work!

lacker

Overall it looks good - just some English nitpicks, and also the float conversion thing seems slightly buggy.

lacker · 2016-10-10T17:00:16Z

docs/docs/composition-vs-inheritance.md

+
+Some components don't know their children ahead of time. This is especially common for components like `Sidebar` or `Dialog` that represent generic "boxes".
+
+We recommend that such components include a prop called `children` in their output:


this sentence was a bit unclear to me - maybe "We recommend that such components use the special children prop to pass children elements directly into their output."

lacker · 2016-10-10T18:10:22Z

docs/docs/lifting-state-up.md

+next: composition-vs-inheritance.html
+---
+
+Often, several components need to reflect the same changing data. We recommend to lift the shared state up to their closest common ancestor. Let's see how this works in action.


recommend to lift -> recommend lifting

lacker · 2016-10-10T18:10:54Z

docs/docs/lifting-state-up.md

+
+Next, we will create a component called `Calculator`. It renders an `<input>` that lets you enter the temperature, and keeps its value in `this.state.celsius`.
+
+Additionally it renders the `BoilingVerdict` for the current input value.


Additionally -> Additionally,

lacker · 2016-10-10T18:16:30Z

docs/docs/lifting-state-up.md

+
+  handleChange(e) {
+    this.setState({
+      celsius: parseFloat(e.target.value)


I think this actually leads to some buggy behavior. Try typing "0.123" on codepen, and then slowly delete it one character by one character - it seems like when you delete the "1" it also deletes the "." and the cursor jumps to the beginning. The problem is that parseFloat + toString is lossy parseFloat("0.").toString() is just "0". So I think the prop actually has to be the string, or something like that. Or maybe you could only accept ints?

One string wouldn't be enough because we wouldn't know which of the fields contains an invalid input. I used to keep two separate strings but this was distracting from the point of this tutorial so I removed that.

This is kind of the edge case that the tutorial inherently has (due to lossy conversions) but most apps don't (because most apps aren't temperature calculators). I'm worried that if we care about fixing this, we'd also have to either change the example to be lossless or add quite a bit of code to achieve the exact behavior we want.

You can't really protect against floats by limiting input to ints because they'll be floats after conversion. And rounding them up is going to look confusing.

I think I'll just add a note about this that explains the tradeoff.

Also apparently it's a bug in React 😢 #7253

gaearon · 2016-10-11T20:58:11Z

Updated both, ready for the final review.

Composition vs Inheritance

Lifting State Up

lacker · 2016-10-11T21:12:22Z

LGTM

facebook-github-bot added GH Review: review-needed CLA Signed labels Oct 7, 2016

suchipi reviewed Oct 7, 2016

View reviewed changes

facebook-github-bot added the CLA Signed label Oct 8, 2016

nashio reviewed Oct 8, 2016

View reviewed changes

facebook-github-bot added the CLA Signed label Oct 8, 2016

gaearon added 2 commits October 8, 2016 15:46

Composition vs Inheritance

4472e22

Small tweaks

300706d

facebook-github-bot added the CLA Signed label Oct 8, 2016

gaearon added 2 commits October 8, 2016 16:00

Moar codepen links

5b12f44

Make it less aggressive

ac952ba

Add Lifting State Up

2ffa931

gaearon changed the title ~~Composition vs Inheritance~~ "Composition vs Inheritance" and "Lifting State Up" Oct 8, 2016

facebook-github-bot added the CLA Signed label Oct 8, 2016

Simplify Lifting State Up

0e7409d

samit4me reviewed Oct 9, 2016

View reviewed changes

Remove radix

e124a1c

kof reviewed Oct 9, 2016

View reviewed changes

draco reviewed Oct 9, 2016

View reviewed changes

gaearon added 2 commits October 9, 2016 16:57

s/celsium/celsius

00a43df

Consistently use classes to make it simpler

4fa2c59

aweary added the Component: Documentation & Website label Oct 10, 2016

facebook-github-bot added the CLA Signed label Oct 10, 2016

gaearon mentioned this pull request Oct 10, 2016

Merge Component Lifecycle guide into React.Component reference lacker/react#1

Merged

lacker reviewed Oct 10, 2016

View reviewed changes

gaearon added 2 commits October 10, 2016 20:45

Address minor nits

aa6b9ed

Update to a sensible behavior

4f6d48f

gaearon merged commit 7544880 into facebook:new-docs Oct 11, 2016

gaearon deleted the composition-over-inheritance branch October 11, 2016 22:08


		Props and composition give you all the flexibility you need to customize a component's look and behavior in an explicit and safe way. Remember that components may accept arbitrary props, including primitive values, React elements, or functions.

		If you want to reuse some common functionality between components that is not related to the UI, we suggest extracting it into a plain JavaScript module, and importing and using that module from the components.


		Our new requirement is that, in addition to a Celsius input, we provide a Fahrenheit input, and they are kept in sync.

		We can start by extracting a `TemperatureInput` component from `Calculator`. We will add a new `scale` prop to it that can either be `"C"` or `"F"`. We will also rename the `celsium` state variable to `temperature` because it may now represent a value in either scale:


		Some components don't know their children ahead of time. This is especially common for components like `Sidebar` or `Dialog` that represent generic "boxes".

		We recommend that such components include a prop called `children` in their output:


		Next, we will create a component called `Calculator`. It renders an `<input>` that lets you enter the temperature, and keeps its value in `this.state.celsius`.

		Additionally it renders the `BoilingVerdict` for the current input value.

"Composition vs Inheritance" and "Lifting State Up" #7920

"Composition vs Inheritance" and "Lifting State Up" #7920

Conversation

gaearon commented Oct 7, 2016 • edited Loading

Choose a reason for hiding this comment

orlando commented Oct 7, 2016

gaearon commented Oct 8, 2016 • edited Loading

suchipi commented Oct 8, 2016

samit4me commented Oct 8, 2016

nashio Oct 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anhuiliujun commented Oct 8, 2016

gaearon commented Oct 8, 2016

gaearon commented Oct 8, 2016

gaearon commented Oct 8, 2016 • edited Loading

gaearon commented Oct 8, 2016 • edited Loading

Lifting State Up

gaelollivier commented Oct 8, 2016

gaearon commented Oct 8, 2016

gaearon commented Oct 8, 2016

gaelollivier commented Oct 8, 2016 • edited Loading

gaearon commented Oct 8, 2016

gaelollivier commented Oct 8, 2016 • edited Loading

suchipi commented Oct 8, 2016

gaearon commented Oct 8, 2016

gaearon commented Oct 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kof Oct 9, 2016 • edited Loading

Choose a reason for hiding this comment

samit4me Oct 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

draco Oct 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon commented Oct 9, 2016 • edited Loading

optimatex commented Oct 9, 2016

lacker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon commented Oct 11, 2016

Composition vs Inheritance

Lifting State Up

lacker commented Oct 11, 2016

gaearon commented Oct 7, 2016 •

edited

Loading

gaearon commented Oct 8, 2016 •

edited

Loading

nashio Oct 8, 2016 •

edited

Loading

gaearon commented Oct 8, 2016 •

edited

Loading

gaearon commented Oct 8, 2016 •

edited

Loading

gaelollivier commented Oct 8, 2016 •

edited

Loading

gaelollivier commented Oct 8, 2016 •

edited

Loading

gaearon commented Oct 9, 2016 •

edited

Loading

kof Oct 9, 2016 •

edited

Loading

samit4me Oct 9, 2016 •

edited

Loading

draco Oct 9, 2016 •

edited

Loading

gaearon commented Oct 9, 2016 •

edited

Loading