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

FSC post #70

Merged
merged 3 commits into from
Mar 7, 2017
Merged

FSC post #70

merged 3 commits into from
Mar 7, 2017

Conversation

jackfranklin
Copy link
Owner

No description provided.

@markerikson
Copy link

Looks pretty good. My main concern is the "performance" section. The phrasing in the React 0.14 announcement has been confusing people since it was published. SOOOO many people think that FSC's are already optimized, but per this tweet from Dan, they aren't: https://twitter.com/dan_abramov/status/832975119224012800 . The closest they've come so far was the discussion that resulted in the creation of React.PureComponent : facebook/react#6914 .

Since FSC's aren't actually optimized yet, in a way they're less performant than class components, because you can't implement shouldComponentUpdate.

That said, this other tweet from Dan says they do still plan some optimizations post-Fiber: https://twitter.com/dan_abramov/status/823818032942379008 .

So, might be worth throwing a couple more caveats into that section regarding current behavior and future plans.

@markerikson
Copy link

markerikson commented Mar 4, 2017

Also, while the comments about defining event handlers in FSC's/render are valid, I do think the perf concerns are somewhat overblown (as a general observation, not just this post). Yeah, you shouldn't make a habit of doing so everywhere, but it's not something that should immediately wiped out of a codebase just because it's wrong.

@jackfranklin
Copy link
Owner Author

@markerikson thank you! I think I agree with you that on both points that I could clarify that a little more. I agree with you RE perf optimisations of inline funcs not making a big diff, but I can probably make that clearer in the post.

@jackfranklin jackfranklin force-pushed the functional-stateless-components branch from b36d91a to 4f6ad5b Compare March 4, 2017 22:58
@jackfranklin
Copy link
Owner Author

@markerikson I updated the content to reflect your comments, which I agree with entirely. Thank you so much!

@markerikson
Copy link

Sure. A few other minor bits of feedback:

  • Might want to mention that you can have "stateless" class components as well, just for clarity. There's some articles that discuss the differences in terminology at https://github.com/markerikson/react-redux-links/blob/master/react-component-patterns.md .
  • Per event handlers: pulling the callback outside the FSC is fine if it only takes the args it's called with, but often you want to refer to captured props (such as () => props.doSomething(props.id) ). In that case, you have to declare it inside the FSC.
  • Typo: "wreally"
  • Typo: the last line where you mention me starts with a "_". That supposed to be italicized? If so, it's missing the closing underscore.

As a final thought: I think the primary benefit of FSCs is simplicity, and to me they act as a visual signal: "this component is solely props in, rendered UI out". If I see a class component, I do have to scan through to see what lifecycle methods it may be using, and what callbacks it may have. If I see an FSC, I know it isn't doing anything fancy. There are definitely times I'll write a stateless class component so I can define callback methods as class properties (especially if I'm passing prop values into a callback prop), but I'll write FSCs to signal that "this is a very straightforward rendering component".

@jackfranklin
Copy link
Owner Author

@markerikson thank you! That last point is a really good one, I've added it to the post :)

@jackfranklin jackfranklin merged commit aa04dd8 into gh-pages Mar 7, 2017
@jackfranklin jackfranklin deleted the functional-stateless-components branch March 7, 2017 10:57
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

Successfully merging this pull request may close these issues.

None yet

2 participants