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

Move when the callback of setState/forceUpdate is called #593

Closed
wants to merge 3 commits into from

Conversation

wyze
Copy link
Contributor

@wyze wyze commented Mar 17, 2017

This aims to solve #556. Not sure how I feel about the loop inside a loop. This does make the test pass though, so it should be a decent place to start from and iterate on if need be.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.695% when pulling b78a41d on wyze:issue-556 into 6c7e70c on developit:master.

@developit
Copy link
Member

Nice, this is clever. I'm not too worried about the nested loop since this only happens at the very end of each render pass, and often the number of pending component mounts there is small.

I'm curious though - does this break setState() callbacks after the first mount? Seems like that loop is only for just-mounted components, whereas the setState() callback needs to be invoked for any update.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.699% when pulling d9f1785 on wyze:issue-556 into ac61ab9 on developit:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 04f5619 on wyze:issue-556 into ca2d921 on developit:master.

@NekR
Copy link
Collaborator

NekR commented Apr 18, 2017

From linked issue:

I'm not sure if this is a bug?It will be error when access domNode of child component in callback.

Why callback to setState supposed to guarantee that everything will be mounted by that point? Is it the purpose of that callback?

From React docs:

Instead, use componentDidUpdate or a setState callback (setState(updater, callback)), either of which are guaranteed to fire after the update has been applied. If you need to set the state based on the previous state, read about the updater argument below.
...
The second parameter to setState() is an optional callback function that will be executed once setState is completed and the component is re-rendered. Generally we recommend using componentDidUpdate() for such logic instead.

For me, this only says that callback is guaranteed to be called when this.state is updated and component has been re-rendered.

If this fix hurts something else or adds additional complexity then I don't it's worth it. If fix is going to be totally safe, then I'm okay with it.

Also, it's a breaking change, so for Preact 9.

@NekR
Copy link
Collaborator

NekR commented Apr 18, 2017

I mean, if it was merged prior Preact 8 went to latest then it could in v8, but now it's too late.

@developit
Copy link
Member

Agreed - if this ends up changing that timing we'd want a major bump. That said, I don't think major bumps are going to have the same scheduling and buildup going forward as they have in the past, they're just backwards-incompatible changes.

@wyze wyze closed this Nov 2, 2018
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.

5 participants