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

Make components stateless #33

Closed
mxstbr opened this issue Dec 22, 2015 · 5 comments
Closed

Make components stateless #33

mxstbr opened this issue Dec 22, 2015 · 5 comments
Assignees
Milestone

Comments

@mxstbr
Copy link
Member

mxstbr commented Dec 22, 2015

Remove all classes and make all components pure functions. E.g. turn

class Example extends Component {
    render() {
        return (
            <h1>Hello {this.props.name}!</h1>
        );
    }
}

into

function Example(props) {
    return (
        <h1>Hello {props.name}</h1>
    );
}

Positives

  • Performance improvement (src)
  • Not possible to use setState(), which is an anti-pattern anyway so we help to keep devs from using it. Since we're using Redux, everything should flow through a reducer.
  • Might be better unit testable, have to look into it.

Negatives

  • No component lifecycle methods, which might confuse newcomers, so that's another thing we'd have to explain in the docs.

Is the more explanation in the documentation worth it? I think so.

@mxstbr mxstbr changed the title Make as many components as possible stateless Make components stateless Dec 22, 2015
@mxstbr mxstbr added this to the v3.0 milestone Dec 24, 2015
mxstbr added a commit that referenced this issue Dec 25, 2015
See issue #33 for more information as to why I changed all components to stateless ones.

Closes #33
@mxstbr mxstbr mentioned this issue Dec 25, 2015
68 tasks
@mxstbr mxstbr self-assigned this Dec 25, 2015
@hampusohlsson
Copy link

It is hard to completely get rid of the lifecycle methods, as you probably will need them for some components. A better approach might be to make the distinction between smart and dumb components.

@mxstbr
Copy link
Member Author

mxstbr commented Dec 27, 2015

@hampusohlsson that's a great link, thanks! How would you reflect that split in the application structure?

@yjcxy12
Copy link
Contributor

yjcxy12 commented Dec 27, 2015

Not really sure if this is good idea. As discussed above, getting rid of all the state and lifecycle methods is not easy and sometimes not necessary. (e.g. a Countdown component might need secondsLeft state, but parent components don't need to be aware this state)

Separating "smart" and "dumb" component can be done putting them into different folders - "containers" for "smart" components and "components" for "dumb" components.

@mxstbr
Copy link
Member Author

mxstbr commented Dec 27, 2015

👍 I really like that approach.

What about pages - they are stateful components, but I feel like they should be in their own folder?

I'm closing this issue in favor of #27, please comment there!

@lock
Copy link

lock bot commented May 30, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators May 30, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants