Lifecycle methods: drop the 'component'. just 'willMount'. it's cleaner #40

Closed
Gregoor opened this Issue Sep 20, 2015 · 16 comments

Projects

None yet

8 participants

@Gregoor
Gregoor commented Sep 20, 2015

Pardon the stupid joke.

I was curious what was the rationale (if there is one) behind having 'component' in the lifecycle methods. To me it seems unnecessary as they are clearly in the context of a component. What would make sense to me though is grouping the lifecycle methods like this:

class Example extends React.Component {

  lifecycle: {
    willMount() {...},
    didMount() {...},
    willReceiveProps() {...},
    //especially weird that shouldComponentUpdate is the only method that has 'component' in the middle
    shouldUpdate() {...},
    willUpdate() {...},
    didUpdate() {...},
    willUnmount() {...},
  }  

}
@motiz88
motiz88 commented Sep 20, 2015

@Gregoor is that idiomatic JS though? I don't think I've seen methods being grouped like this anywhere. Note that exampleInstance.lifecycle.willMount now does the wrong thing by default and would need to be invoked with Function.prototype.apply or .call, unless some preprocessing a la React.createClass was also performed (and I think it's clear at this point that that's not where React is headed).

In the React.createClass days I used to split complex specs into separate objects and merge them back before providing them to React. You could similarly compose your component class from discrete parts using ES2015+ facilities (inheritance, decorators and what not) then export a well-behaved React component.

Personally I don't believe React should introduce its own variations on JavaScript class semantics, unless they're very widely applicable and already have a good chance of landing in a future ES spec.

@Gregoor
Gregoor commented Sep 20, 2015

You're raising good points, thanks. The grouping is something that was born out of frustration about codebases I had to work with where the methods were all over the place. But I guess you're right and the benefit (clarity) doesn't outweigh the cost (having to rebind the methods) in this case.
Though I do not think it's completely non-idiomatic since grouping methods inside of objects feels pretty JSy to me 😋

But I would still like to know if there is a chance that the methods will be renamed?

@rozzzly
rozzzly commented Sep 22, 2015

Eh, I see you're reasoning; I certainly agree that it's a little verbose/superfluous. However, I would advocate against any such changes for two reasons:

  • Even assuming there was a gentle transition where this was deprecated (throwing a warning) for a few versions, it would be a major breaking change. And not only for the source code, Imagine the following scenario:

    It's September 21st, 2016. I've been hearing about this awesome react thing for a while now. It has grown a really robust community and everybody loves it yadayada... I want to learn it too! So I go to the official react site. The official docs show all of the most current api's with @Gregoor 's proposed changes now fully adopted. A lot of the other first-class resources at the top of Google also show this syntax. I follow the tutorials and everything is going great. [Now here is the issue] I had a question about some more advanced functionality not sufficiently covered in the official/popular & updated docs so I go to Google. That leads me to some blog post, GitHub Issue, or an answer on StackOverflow, etc, etc. that does a great job of telling me what I needed to know. I test the code out, but it doesn't work. After a bunch of digging around I find that because post was written in May of 2015, It uses the old api. Therefor I need to go rewrite parts of the code.

    It's not a huge fix, but I had to go through this entire rigmarole (word of the day 😉) to get this working whereas before just copy+paste would have worked

    This is always an issue for any change to any API, but seeing as this one is rather trivial, why put people through that? We're already doing so with the move to es6 e.g. React.createClass vs class MyComponent extends React.Component, and

    ...
    constructor(props) {
      super(props);
      this.state = {
        someVariable: 'someValue'
      };
    }
    ...
    

    instead of the antiquated

    ...
    getInitialState: function() {
      return {
        someVariable: 'someValue'
      };
    }
    ...
    

    I just can't justify that over something so minor, that isn't really causing any problems.

  • Breaking compatibility/dependencies. This would result in many packages becoming incompatible do to the change. If this were to happen, future me couldn't necessarily use some of the newer packages if I had a legacy codebase, or visa-versa. Especially mixins/decorators. I'm sure there would be some unforeseen issues with the handling of nested members when using a mixin or decorator.

  • @Gregoor, you say

    Grouping methods inside of objects feels pretty JSy to me.

    It's certainly a popular design pattern—or rather it has been—but this isn't one react is structured around. Considering how we're moving to es6, I think grouping those methods in an object would be quite counterintuitive; people are going to be wanting to use class MyObj { } not MyObj = { ... }


All in all, @Gregoor, I am glad you asked this question. It brings up some interesting points for consideration. Whilst I think your proposal would be neither advantageous, nor very practical, I believe it does beg the question of how the community should design new additions to the react API as we go forward in the coming months and years. Assuming one is using class MyComponent extends React.Component { ... }, functions named following component______() (e.g. componentWillUpdate()) are very redundant indeed. I don't know what's on the horizon for react, but how we define the API for those new changes—considering the adoption of es6—must be something we keep in mind.

@rozzzly
rozzzly commented Sep 22, 2015

@motiz88 I don't follow you when you say:

Note that exampleInstance.lifecycle.willMount now does the wrong thing by default and would need to be invoked with Function.prototype.apply or .call
What did you mean by that?

And in regard to you not seeing grouping like he used as a design pattern, I'd like to add: I've definitely seen it before but not really for Class/Object definitions, more like passing an object with your custom handlers/etc to a jQuery plugin. I don't care to search any examples, but it is certainly out there.

And I agree, that's not where the community is moving. We're moving away from those semantics and towards something more OOP like you'd see in, um say, java. (thankgod)

@motiz88
motiz88 commented Sep 22, 2015

@rozzzly Invoking exampleInstance.lifecycle.willMount would set the function's context (this) to the lifecycle object and not to exampleInstance. Therefore calls like this.setState() would not work inside willMount - unless React knew to explicitly rebind the lifecycle methods to run in the context of exampleInstance, either at each call site by apply/call, or ahead of time somehow using bind. Both options are not pretty in terms of changes to React's internals and API, particularly when considering backwards compatibility.

In userland, though, if it fits your project, you can always come up with your own abstractions/helpers and come very close to @Gregoor's suggested format.

@Gregoor
Gregoor commented Sep 22, 2015

Thanks a lot for the detailed answer. Reading it was the opposite of a rigmarole (am I doing this right?).

I do see that lifecycle methods not being called would be tricky to debug in a distant future, where all the deprecation messages would be gone. But then again, if someone only programs by copy pasting without even googling the used methods (which would bring up plenty of results informing about the change), that person would have it coming. 😛
I think its one of the major strengths of React and its team that you're doing these breaking ES6 changes right now, because they make sense (same goes for the breaking refs-change). And while this change might be even more breaking, I think it makes sense. Some beginners might be confused by the change, but some beginners might also be more intrigued by it.

Anyway, it's your baby and I will always love you for open-sourcing it, no matter what. Its probably a minor thing that makes more sense in my head because I don't have to deal with the negative consequences of it 😆

@spicyj
Member
spicyj commented Sep 22, 2015

We've talked about this. Basically, it comes down to this:

We value clarity and legibility over terseness. The "component" prefix makes things easier to grep for and makes it clearer that these are magic methods that React interprets specially. There's also a cost to a breaking change (as @rozzzly mentioned) which we'd rather avoid if we can.

For these reasons, we don't plan to change this.

@spicyj spicyj closed this Sep 22, 2015
@Gregoor
Gregoor commented Nov 3, 2015

thanks to @cpojer I can invalidate the "cost of API change"-argument.
http://felix-kling.de/esprima_ast_explorer/#/ssYCjtA7z5

The grep and clarity arguments doesn't hold up in my opinion. Same would have to be true for render.

Still, it's minor so feel free to ignore this. I was just looking for a reason to play around with codemods.

@sebmarkbage
Contributor

Life-cycles are also considered "escape hatches", edge cases that you shouldn't normally need to use. They provide hooks for frameworks or edge cases rather than being encouraged in normal use. It's a classic framework trick to name discouraged methods with long names. :) Not sure how valid but seems to work.

They also share the namespace with user code so they're less likely to collide if they're more explicit.

It's probably safe to apply a codemod but even a codemod requires work/testing so we try to keep those to a minimum when it buys us something substantial.

@Gregoor
Gregoor commented Nov 3, 2015

Fair enough, with that in mind I just looked through my code bases and found that most of the time when I use lifecycles, it's to interop with a non-React lib.

Thanks! :)

@dzannotti

Probably OT but i always felt the same way as @Gregoor
@sebmarkbage given a route driven application, and given that i feel the same way about "escape hatches" i have yet to find a better pattern to post an api call to the dispatcher when a specific route is hit (beside relay that is), what's your suggestion?

@hax
hax commented Nov 6, 2015

@sebmarkbage Maybe Symbols is better than long names for such usage 😃

@me-andre

I would like to add that if an object from which you construct a component is a rich object (it can be literally anything with a render() method) it makes sense to have React-related methods prefixed to avoid collisions.

@Gregoor
Gregoor commented Dec 23, 2015

Well if you would be supplying a "rich object" to React, I would imagine that you're violating single responsibility and should rather go with composition in that case.

@me-andre

@Gregoor , I agree. Personally I tend to apply composition whenever possible. However, I don't see a reason to reduce available options which the current API allows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment