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

Fixing onrenderhook chain #266

Closed
wants to merge 1 commit into from
Closed

Fixing onrenderhook chain #266

wants to merge 1 commit into from

Conversation

dxlbnl
Copy link
Contributor

@dxlbnl dxlbnl commented Jan 25, 2017

Fixes: #263

I've renamed _root to _parent when nesting components. _renderHooks should be called when components are mounted, but it isn't being emptied. So adding _renderHooks to a component's parent should solve this issue.

_root is never being used anywhere else, and I don't think it should be needed?

@dxlbnl
Copy link
Contributor Author

dxlbnl commented Jan 25, 2017

Hmm, so there's another usecase where there's no onrender on the containing element.

So there should be a parent-update chain.

@Rich-Harris
Copy link
Member

Thanks for looking into this! Haven't had a chance to investigate yet myself, so the help is very much appreciated

@dxlbnl
Copy link
Contributor Author

dxlbnl commented Jan 26, 2017

I've been looking into it a bit further, But I don't think I can make a decision.
The error (why the checks are failing) is due to that the root component is not mounted yet, when the onrender of ParentWidget -> Widget is being called.

I know of #40, but maybe onrender should be called oncreate. But then it shouldn't guarantee that it is mounted in the document. There could be a onmounted event, to handle stuff when it can be guaranteed to be mounted in the document.

So test onrender-fires-when-ready-nested could be counting on unspecified behaviour.

I'll also add my test case to the tests.

@Rich-Harris
Copy link
Member

I've taken another run at this --> #273

@Rich-Harris
Copy link
Member

Merged #273 so will close this — thanks again

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.

2 participants