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

[WIP] Reduces lifecycle for environments with no dom #326

Conversation

pragmaticivan
Copy link
Contributor

@pragmaticivan pragmaticivan commented Dec 5, 2017

closes #318

@robframpton
Copy link

Hey @pragmaticivan,

Maybe to reduce the amount of calls to isDom, we can call it once in the Component constructor and assign it to an instance property? Maybe call it domExists or something like that.

@pragmaticivan
Copy link
Contributor Author

pragmaticivan commented Dec 5, 2017

hey @Robert-Frampton just sent some changes applying as you've suggested

@pragmaticivan pragmaticivan changed the title Reduces lifecycle for environments with no dom [WIP] Reduces lifecycle for environments with no dom Dec 6, 2017
@pragmaticivan
Copy link
Contributor Author

Changing that to WIP for more discussion about having disposed/dispose also willAttach maybe seems to be a weird name for something that will not attach anything on SSR since there's no Dom.

@robframpton
Copy link

That's a good question. I wonder why React does it that way.

It seems that there might be just as much of a chance of things breaking by firing willAttach on the server. I wouldn't be surprised if a component did something DOM related during that lifecycle step.

@pragmaticivan
Copy link
Contributor Author

Was also thinking about leaving created since it represents the event in the constructor.

Wonder if you guys have any feedback on that Topic: cc @mthadley @bryceosterhaus

@mthadley
Copy link

mthadley commented Dec 6, 2017

@pragmaticivan I think that it's generally a good idea to do any DOM related work inside of attached and detatched, instead of created. But I think you will find that sometimes developers will end up putting that kind of logic in created as well, even if they shouldn't.

@pragmaticivan
Copy link
Contributor Author

pragmaticivan commented Dec 6, 2017

I agree, but wonder if that's not a good time to create a "Best Practice Guide" for developing metal components where it should be performed on SSR and client, and maybe assume that they won't use created for DOM related stuff.

@eduardolundgren
Copy link
Contributor

Even though I like the concept of simplifying server-side rendering by avoiding manual checks by the developer, lifecycle changes can mess things we can't predict. If you guys decide in moving forward discuss carefully the effects.

@jbalsas
Copy link
Contributor

jbalsas commented Dec 11, 2017

I actually find this PR quite confusing. If I'm reading it correctly, the component lifecycle methods are being executed, but we won't be dispatching the events or will be exiting those methods quick.

If we wanted to do this, I think it should be implemented somewhere else, so the methods on Component are not invoked at all. That would allow us to properly document SSR lifecycle. At the same time, if done this way, the risks @eduardolundgren is mentioning are better understood since we'd see calls disappear (risky). Just adding flow logic to the event emitters like in this PR seems quite harmless, which may not be the case.

@pragmaticivan
Copy link
Contributor Author

All good, I will close this PR and will do some research. Thanks for the feedback o/

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.

Reduce lifecycle for Server Side Rendering
5 participants