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

Enforce component lifecycle hook order #137

Merged
merged 1 commit into from
Sep 22, 2017
Merged

Enforce component lifecycle hook order #137

merged 1 commit into from
Sep 22, 2017

Conversation

RobbieTheWagner
Copy link
Contributor

As a followup to my previous route hook order PR (#124) I wanted to get one started for components, and models will be next after this 😄

I’m enforcing that component hooks be called, more or less, in execution order. There is some weirdness to consider with certain hooks executed on both initial render and rerender, but I believe this order makes sense.

@RobbieTheWagner
Copy link
Contributor Author

@michalsnik @kevinkucharczyk what do you guys think of this? It should be basically the same implementation as the routes one, just different hooks. Would love to get this in, so we can enforce hook order in both routes and components 😄

Copy link
Member

@michalsnik michalsnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @rwwagner90, thanks for putting your effort into this! It looks good to me and I agree with the proposed order :)

However I would also like to hear what @kevinkucharczyk or @Turbo87 might have to say about this proposal.

@RobbieTheWagner
Copy link
Contributor Author

@michalsnik great, thanks for taking a look! I look forward to hearing what the other guys think 😃

@RobbieTheWagner
Copy link
Contributor Author

@kevinkucharczyk @Turbo87 do you guys have any thoughts on this? Would love to get it merged in 😄

@Turbo87
Copy link
Member

Turbo87 commented Sep 13, 2017

I've stopped using the order rules since they often caused false positives for example for empty functions that are supposed to be overwritten. Also I think the rules would probably work a lot better if they had support for --fix.

tl;dr I don't use those rules at the moment so I'm fine with whatever you decide.

@RobbieTheWagner
Copy link
Contributor Author

@Turbo87 I would be glad to help try to implement a --fix for the ordering rules, once I get all the lifecycle hook ordering ones in. Models are after this.

@RobbieTheWagner
Copy link
Contributor Author

@michalsnik since @Turbo87 is okay with this, and it's been approved, can we merge?

@michalsnik
Copy link
Member

@Turbo87 support for empty functions in order have been added in #108
@rwwagner90 I think it's good to go :)

@michalsnik michalsnik merged commit 29c56c6 into ember-cli:master Sep 22, 2017
@RobbieTheWagner RobbieTheWagner deleted the component-order branch September 27, 2017 10:47
@RobbieTheWagner
Copy link
Contributor Author

@michalsnik would it be possible to get a release with these changes please? I also looked at the releases on here and it looks like 4.3.0 was the last officially tagged "release" and 4.4 and 4.5 were just tags pushed up. Wanted to make you aware of that in case we want them to show as official releases on GitHub.

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

Successfully merging this pull request may close these issues.

None yet

3 participants