Skip to content

Conversation

@sophiebits
Copy link
Collaborator

Based off of @ngavalas's #93. Probably not quite mergeable yet but I wanted to send this out tonight and get some feedback.

The end of ReactUpdates-test.js is probably most illuminating for seeing how this works.

ngavalas added 4 commits June 14, 2013 16:23
This commit adds an optional callback as a second argument to
`setState`, to be called after `setState` runs.

We never guarantee synchronous execution of `setState`, and as per
@phunt, we don't want to make that guarantee because we may eventually
batch calls to `setState`.  @jwalke agrees with him.
Change api docs to reflect presence of the new argument.  In addition,
callback was change to require only a "truthy" value.
Small problem with markdown syntax in syntax-highlighted block.
All public facing {set,replace,force}{props,state} methods now support
callbacks.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should throw this in a separate module that is webworker-safe and supports a better RAF polyfill (@benjamn has a good one)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Supposedly setTimeout exists in a web worker, but I just realized that this reference to window will probably fail. Happy to use a polyfill if one exists.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just splitting it out into a separate module is fine for now. Him or I can drop his polyfill in there later.

Copy link
Contributor

Choose a reason for hiding this comment

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

This appears unused

@petehunt
Copy link
Contributor

@spicyj if you need some help with this lmk and I can stack a commit on top of this or something :) I'm really excited about landing this!

@ashwinb
Copy link

ashwinb commented Jun 24, 2013

@petehunt++ -- i am also very excited for this update.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@yungsters
Copy link
Contributor

This is great. Along the lines of your TODO, have you considered queueing mounting, also?

@sophiebits
Copy link
Collaborator Author

Going to leave mounting as synchronous for now I think but I need to add a callback to renderComponent because that calls setProps half the time.

@sophiebits
Copy link
Collaborator Author

@petehunt @yungsters @jordwalke Took another pass at it – I like this version a lot better. For now, I'm only batching things together when they're in a block like this:

ReactUpdates.batchedUpdates(function() {
  component.setState(...);
  component.setState(...);
});

This means that the event handlers have batched updates but nothing else does yet. I wrote ReactUpdates such that it should be easy to make this defer using rAF in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit.

if (A &&
    B &&
    C) {

@yungsters
Copy link
Contributor

The revised version makes a ton of sense. I love the direction and agree with the TODO comments. 👍

@sophiebits
Copy link
Collaborator Author

(fixed spacing nits)

@petehunt
Copy link
Contributor

if @yungsters is good, I'm good.

@ngavalas
Copy link
Contributor

Lovin' this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, forceUpdate.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we should try/finally this, right?

@petehunt
Copy link
Contributor

petehunt commented Jul 2, 2013

@spicyj looks like you've got a failing test; can you take a look? I tried the naive approach of surrounding with batchedUpdates() but it didn't work.

The end of ReactUpdates-test.js is probably most illuminating for seeing how this works.
petehunt added a commit that referenced this pull request Jul 3, 2013
Batch together calls to setState, setProps, etc
@petehunt petehunt merged commit 3093a47 into facebook:master Jul 3, 2013
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.

7 participants