Skip to content
This repository has been archived by the owner on Aug 31, 2021. It is now read-only.

Functional components #162

Closed
wyuenho opened this issue Apr 6, 2017 · 10 comments
Closed

Functional components #162

wyuenho opened this issue Apr 6, 2017 · 10 comments

Comments

@wyuenho
Copy link
Contributor

wyuenho commented Apr 6, 2017

Related to #161

Let's ID a list of components we have a high degree of confidence we'll never need the ref, then convert them to functional components, or if we can, PureComponents, so they can be faster now without having to wait for Fiber.

@tajo
Copy link
Contributor

tajo commented Apr 6, 2017

Everything that accepts only primitive types (no arrays or objects) can be PureComponent.

@wyuenho
Copy link
Contributor Author

wyuenho commented Apr 6, 2017

If that's the case, do we still want functional components?

@tajo
Copy link
Contributor

tajo commented Apr 6, 2017

Yes, you can use the same optimization for functions too - see pure. It's even more idiomatic.

In fact, we could keep all components functional and in cases you really need refs you can use toClass HOC.

There are many more sweet helpers for functional components in recompose but we can't use them anymore...

@wyuenho
Copy link
Contributor Author

wyuenho commented Apr 6, 2017

1 thing at a time. There are way too many libraries and APIs we have to wrap our heads around already. How about this: PureComponents for what we can first, functional components for simple stupid stateless shit that still remain? Prepending "Pure" to "Component" seems a lot less drastic to me.

@tajo
Copy link
Contributor

tajo commented Apr 6, 2017

export default createComponent(styles, FunctionalComponent); 

to

export default createComponent(styles, pure(FunctionalComponent)); 

is "drastic" and hard to wrap our heads around ?

functional components for simple stupid stateless shit that still remain

Frankly, I don't know what "stupid stateless shit" is anymore. I thought it's stuff like heading or button or almost any cf-ui component. Apparently not. I still think that we should not promote usage of refs by default (if that was the main argument for classes). In case you need them, you could use toClass().

@wyuenho
Copy link
Contributor Author

wyuenho commented Apr 6, 2017

I was referring to the changing of classes to functions. We do not promote ref usage. I don't know what will remain, just there will be plenty of simple stupid stateless shit like ModalHeader. ("shit" was added for effect, don't take it seriously)

@jculvey
Copy link
Contributor

jculvey commented Apr 6, 2017

A couple of thoughts:

  • Let's base this decision on developer experience and avoiding future rework, not perf. If we're going to make perf decisions, we should measure before and after a fix and base the decision on that.
  • There's no harm in making a list like this. At the very least, it will help us really identify the use cases for refs and think some actual examples.
  • There's some truth to keeping a low concept overhead. A lot of little simple things can add up to a really complicated thing if you let it. I definitely support using helpers and libraries that make our lives way easier, and we've made some really good picks so far. But we should at least ask if we're spending our concept budget wisely when we explore new things.

@wyuenho
Copy link
Contributor Author

wyuenho commented Apr 7, 2017

I deliberately tried to steer the discussion towards something we can measure, like perf. I don't know how to measure DX and predict the future. We all have different mental models, and I don't buy into any holy war arguments that one way is definitely better than the other. Functions are simple, but it is also because they are too simple, we'll need an extra library to help us do the things we can already do in React.

I can say converting all classes to function wholesale is definitely going to be more work now. If some work is deemed necessary, the only difference is when to do it. I'm not yet convinced it's necessary to convert all classes to functions. I'm also against doing this during the transition to fela. However, if some people want to do it, let's have this discussion now. I'm willing to compromise, and ready to be convinced. The only thing I'm not okay with is introducing drastic, breaking changes without discussion.

@sejoker
Copy link
Contributor

sejoker commented Apr 7, 2017

I am happy with whatever decision we will make here, at this point it's too minor to spend much time on it, please don't forget, we all humans and bike shedding is a thing.

@jwineman
Copy link
Contributor

jwineman commented Apr 7, 2017

I'm hesitant to use 3rd party libraries like recompose for things we can accomplish easily without them. Not all of our components can be converted to stateless functions and there is no reason we need an all or nothing approach.

I'm in favor of converting any component that can be stateless to be stateless. To the developer experience point stateless components:

  • Have a smaller surface area for bugs
  • Are easier to test
  • Easier to reason about

I think we should wait until the Fela migration is complete, make a list of everything that depends on refs like @jculvey said, and convert the rest to stateless.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants