Skip to content

Document ReactDOM.unstable_batchedUpdates #5880

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

Closed
wants to merge 1 commit into from
Closed

Document ReactDOM.unstable_batchedUpdates #5880

wants to merge 1 commit into from

Conversation

graue
Copy link
Contributor

@graue graue commented Jan 19, 2016

Took a stab at documenting this. Let me know what you think.

Fixes #3570

@graue
Copy link
Contributor Author

graue commented Jan 30, 2016

catping

@jimfb
Copy link
Contributor

jimfb commented Feb 4, 2016

We generally don't document the unstable_ features.

Also, FWIW, this function is really only useful to react framework authors (ie. not the general public component authors), so top-level api docs are probably too prominent. Most of the framework authors are tied into github and know how to find the unstable/undocumented functionality.

@graue
Copy link
Contributor Author

graue commented Feb 12, 2016

Can I get a second opinion on this? @spicyj ?

We saw an over 25% speedup in a part of Instagram.com after using this hook, so I disagree with the notion that it's only useful to framework authors. This should be accessible to everyone to eke out perf wins.

@sophiebits
Copy link
Collaborator

25% faster on what sort of operation?

@mridgway
Copy link
Contributor

We used this for batching Flux change events with a lot of success. I agree it shouldn't be documented unless it becomes "stable", but I would argue that it's useful enough to become a top level API.

@sophiebits
Copy link
Collaborator

I don't feel strongly either way about documenting it so I am willing to follow @jimfb's lead in saying no.

We do want to move to automatically batching everything, at which time this hook is moot. Until then it will probably stay where it is.

@graue
Copy link
Contributor Author

graue commented Feb 14, 2016

On rerendering when you navigate to a new post. This is easy to document (I've already done it), will enable people to write more performant apps and ship better UX, and is easily codemodded away if and when automatic batching comes to pass (which is probably at least months away from being in a release, yes?). Why not?

@jimfb
Copy link
Contributor

jimfb commented Feb 14, 2016

On rerendering when you navigate to a new post.

This doesn't help me to understand. Specifically, what operations is the app doing that results in a benefit from batching (and why is the app doing whatever operations it is doing). A single rerender would not benefit from batching.

This is easy to document (I've already done it), will enable people to write more performant apps and ship better UX, and is easily codemodded away if and when automatic batching comes to pass (which is probably at least months away from being in a release, yes?). Why not?

It is not about how easy/hard it is to document - it's about the stability guarantees we're making. If we want to release this as stable, we could consider that, but we shouldn't document something that is unstable_. We also shouldn't release something as stable if we're just going to get rid of it in a couple months.

@graue
Copy link
Contributor Author

graue commented Feb 20, 2016

@jimfb, N>1 Flux stores emit change events. Naively, this leads to N renders. You skipped over my key point as to why the benefits of documenting outweigh the downsides:

is easily codemodded away if and when automatic batching comes to pass (which is probably at least months away from being in a release)

@mxstbr
Copy link
Contributor

mxstbr commented Feb 20, 2016

@graue I'd be interested in a code snippet how you did that.

@graue
Copy link
Contributor Author

graue commented Feb 23, 2016

@mxstbr:

class MyDispatcher extends Dispatcher {
  constructor() {
    super();
    this._actuallyDispatch = Dispatcher.prototype.dispatch.bind(this);
  }
  dispatch(payload) {
    ReactDOM.unstable_batchedUpdates(this._actuallyDispatch, payload);
  }
}
export default new MyDispatcher();

This is from memory, may not be 100% right.

@jayarjo
Copy link

jayarjo commented Aug 26, 2018

We do want to move to automatically batching everything, at which time this hook is moot. Until then it will probably stay where it is.

@sophiebits did you finally move to automatically batching everything?

@gaearon
Copy link
Collaborator

gaearon commented Aug 26, 2018

Not yet.

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.

8 participants