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

Rename createSyncRoot to createBlockingRoot #17165

Merged
merged 2 commits into from
Oct 23, 2019

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Oct 21, 2019

"Sync" is easy to confuse with the Legacy mode. It also might sound too positive.

While I'm renaming createSyncRoot to createBlockingRoot, I also did some find-and-replace in comments and internal naming. Let's standardize on:

  • Legacy Mode: The old one.
  • Blocking Mode: createBlockingRoot
  • Concurrent Mode: createRoot

@acdlite
Copy link
Collaborator

acdlite commented Oct 21, 2019

Can we consider passing an option to createRoot instead, like {disableConcurrentRendering: true}? (This options object already exists for enabling hydration.) I think the original reason we went with a separate API was because we didn't want to expose createBatch to the non-concurrent version. But createBatch no longer exists.

@acdlite
Copy link
Collaborator

acdlite commented Oct 21, 2019

I'm just not convinced "blocking" is a thing people will intuitively understand and I'm worried the additional terminology is more confusing than it's worth.

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 21, 2019

Concurrent Mode with "disabled concurrent rendering" isn't that much easier to explain IMO though — and we need to refer to this concept somehow when discussing adoption strategy.

@sizebot
Copy link

sizebot commented Oct 21, 2019

Size changes (experimental)

No significant bundle size changes to report.

Generated by 🚫 dangerJS against be0ce36

@acdlite
Copy link
Collaborator

acdlite commented Oct 21, 2019

But like what is the difference between "blocking" and "concurrent"? Are they mutually exclusive? Can you have one without the other? Technically legacy is also blocking. Why not createNonConcurrentRoot? I don't get what "blocking" buys us as an additional terminology.

@sizebot
Copy link

sizebot commented Oct 21, 2019

Size changes (stable)

No significant bundle size changes to report.

Generated by 🚫 dangerJS against be0ce36

@acdlite
Copy link
Collaborator

acdlite commented Oct 21, 2019

To clarify, I have two separate objections:

  • Do we need two separate methods, instead of using an option? Given that the end game is to get rid of "blocking" mode and move everyone to concurrent mode.
  • Is it worth adding a new term "blocking" when most people won't know what that means without an extra explanation, at which point we might as well keep using "concurrent."

@acdlite
Copy link
Collaborator

acdlite commented Oct 21, 2019

OTOH I don't think it's any worse than what we already have, so I don't mind if we merge this for now. I would prefer to bikeshed it more before the actual release, though.

@sebmarkbage
Copy link
Collaborator

There are a number of considerations for an options object:

  • Future additions to the api (such as adding createBatch back) that are mode specific.
  • Tree shaking, or clearly defined separate modules (like /legacy), makes it easier to create separate builds for it in the future.
  • It encourages dynamic usage in wrappers. In fact that’s exactly what we do with hydration at FB.

That said, these arguments mostly apply to hydration too and we already said we didn’t want a combinatorial explosion.

@sebmarkbage
Copy link
Collaborator

I mostly suggested it because Sync looked bad in the column. I’m all for bikeshedding this further before release.

However we should decide the name of the column now “Legacy Mode”, “??? Mode”, “Concurrent Mode”.

@acdlite
Copy link
Collaborator

acdlite commented Oct 21, 2019

What I like about "legacy" is that describes the target use case as opposed to the technical characteristics. Maybe we could do the same here. "Compatibility Mode"?

@sebmarkbage
Copy link
Collaborator

Tests failing.

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 23, 2019

Oops. Pushed

@gaearon gaearon merged commit f6b8d31 into facebook:master Oct 23, 2019
@gaearon gaearon deleted the rename-modes branch October 23, 2019 22:04
ecreeth added a commit to ecreeth/concurrent-react-notes that referenced this pull request Oct 24, 2019
- **createSyncRoot** was renamed to **createSyncRoot** in [#17165](facebook/react#17165)
- **unstable_** is not necessary in experimental releases. See [#17146](facebook/react#17146) and [#17108](facebook/react#17108)
@gaearon gaearon mentioned this pull request Mar 29, 2022
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