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

Document legacyness of the return value of ReactDOM.render() #6400

Merged
merged 1 commit into from Apr 7, 2016

Conversation

Projects
None yet
7 participants
@jimfb
Contributor

jimfb commented Apr 1, 2016

Document legacyness of the return value of ReactDOM.render(), as per #6397

@jimfb

This comment has been minimized.

Show comment
Hide comment
@jimfb

jimfb Apr 6, 2016

Contributor

Ping @gaearon @zpao Can one of you review this?

Contributor

jimfb commented Apr 6, 2016

Ping @gaearon @zpao Can one of you review this?

@iamdustan

This comment has been minimized.

Show comment
Hide comment
@iamdustan

iamdustan Apr 6, 2016

Contributor

Would it be premature to add a rule for this to https://github.com/yannickcr/eslint-plugin-react/ start filtering this information out to the community?

Contributor

iamdustan commented Apr 6, 2016

Would it be premature to add a rule for this to https://github.com/yannickcr/eslint-plugin-react/ start filtering this information out to the community?

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Apr 6, 2016

Member

👍 I think it’s a good idea. This would be a breaking change for them tho? If it becomes a default. @iamdustan

Member

gaearon commented Apr 6, 2016

👍 I think it’s a good idea. This would be a breaking change for them tho? If it becomes a default. @iamdustan

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Apr 6, 2016

Member

@jimfb I’m accepting on the condition you address the above comments. Thanks!

Member

gaearon commented Apr 6, 2016

@jimfb I’m accepting on the condition you address the above comments. Thanks!

@jimfb jimfb merged commit 1dc7c58 into facebook:master Apr 7, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

zpao added a commit that referenced this pull request Apr 7, 2016

Merge pull request #6400 from jimfb/return-value-legacy
Document legacyness of the return value of ReactDOM.render()
(cherry picked from commit 1dc7c58)
@jeffbski

This comment has been minimized.

Show comment
Hide comment
@jeffbski

jeffbski Apr 11, 2016

Contributor

This should probably be considered along with ReactDOM.renderToString and ReactDOM.renderToStaticMarkup. Would those necessarily not be able to synchronously return the string markup too (assuming an async render is allowed)?

If that is true, then would they return a promise, observable, or use a new cb parameter to obtain the html?

So whatever is decided for those, maybe a similar mechanism should be allowed for ReactDOM.render for symmetry. Meaning if renderToString and renderToStaticMarkup both return a promise which eventually resolves to the html, maybe render should return a promise which eventually resolves to the component. Likewise if observable or cb mechanism is chosen. This is an alternative to using the ref cb.

It is nice to have symmetry as much as possible in these api's. Developers have enough other things to worry about.

Contributor

jeffbski commented Apr 11, 2016

This should probably be considered along with ReactDOM.renderToString and ReactDOM.renderToStaticMarkup. Would those necessarily not be able to synchronously return the string markup too (assuming an async render is allowed)?

If that is true, then would they return a promise, observable, or use a new cb parameter to obtain the html?

So whatever is decided for those, maybe a similar mechanism should be allowed for ReactDOM.render for symmetry. Meaning if renderToString and renderToStaticMarkup both return a promise which eventually resolves to the html, maybe render should return a promise which eventually resolves to the component. Likewise if observable or cb mechanism is chosen. This is an alternative to using the ref cb.

It is nice to have symmetry as much as possible in these api's. Developers have enough other things to worry about.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Apr 11, 2016

Member

I think renderTo* methods would stay synchronous because they don’t have any updates. They are meant to be called as one-offs, rather then maintain continuously rendering UIs. On the other hand, ReactDOM.render() schedules a reconciliation that can be paused because other important work is happening (e.g. handling a gesture). Incremental reconciliation doesn’t really help on the server because you can’t just show “some parts” and later show “other parts”—once you send the response it’s too late, and in any case the network latency is much bigger than the rendering cost so you “drop frames” anyway.

Member

gaearon commented Apr 11, 2016

I think renderTo* methods would stay synchronous because they don’t have any updates. They are meant to be called as one-offs, rather then maintain continuously rendering UIs. On the other hand, ReactDOM.render() schedules a reconciliation that can be paused because other important work is happening (e.g. handling a gesture). Incremental reconciliation doesn’t really help on the server because you can’t just show “some parts” and later show “other parts”—once you send the response it’s too late, and in any case the network latency is much bigger than the rendering cost so you “drop frames” anyway.

@jimfb

This comment has been minimized.

Show comment
Hide comment
@jimfb

jimfb Apr 11, 2016

Contributor

@gaearon beat me to the punch. His answer is exactly correct. 👍

Contributor

jimfb commented Apr 11, 2016

@gaearon beat me to the punch. His answer is exactly correct. 👍

@jeffbski

This comment has been minimized.

Show comment
Hide comment
@jeffbski

jeffbski Apr 11, 2016

Contributor

ok, thanks. That answers my question.

Contributor

jeffbski commented Apr 11, 2016

ok, thanks. That answers my question.

@zpao zpao added the semver-exempt label Apr 22, 2016

iamdustan added a commit to iamdustan/eslint-plugin-react that referenced this pull request May 3, 2016

iamdustan added a commit to iamdustan/eslint-plugin-react that referenced this pull request May 3, 2016

iamdustan added a commit to iamdustan/eslint-plugin-react that referenced this pull request May 4, 2016

@iamdustan

This comment has been minimized.

Show comment
Hide comment
@iamdustan

iamdustan Jun 6, 2016

Contributor

A quick follow-up that a rule for this has landed in eslint-plugin-react. yannickcr/eslint-plugin-react#586

Contributor

iamdustan commented Jun 6, 2016

A quick follow-up that a rule for this has landed in eslint-plugin-react. yannickcr/eslint-plugin-react#586

@taion

This comment has been minimized.

Show comment
Hide comment
@taion

taion Jan 7, 2018

In light of #11401, attaching a callback ref to the root component may cease to carry the desired semantics here.

As such, should the guidance instead be updated to be to use the callback from ReactDOM.render?

taion commented Jan 7, 2018

In light of #11401, attaching a callback ref to the root component may cease to carry the desired semantics here.

As such, should the guidance instead be updated to be to use the callback from ReactDOM.render?

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jan 7, 2018

Member

Hmm could you clarify what would change?

Member

gaearon commented Jan 7, 2018

Hmm could you clarify what would change?

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jan 7, 2018

Member

(To be clear I don't think there was a team consensus to implement #11401 specifically—it was just one of proposals and IMO a somewhat extreme one.)

Member

gaearon commented Jan 7, 2018

(To be clear I don't think there was a team consensus to implement #11401 specifically—it was just one of proposals and IMO a somewhat extreme one.)

@taion

This comment has been minimized.

Show comment
Hide comment
@taion

taion Jan 7, 2018

I mean "use the optional callback function to ReactDOM.render" instead of "attach a callback ref".

Though this problem would sort of go away if ReactDOM.render returned a promise or something ;)

taion commented Jan 7, 2018

I mean "use the optional callback function to ReactDOM.render" instead of "attach a callback ref".

Though this problem would sort of go away if ReactDOM.render returned a promise or something ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment