Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

AsyncMode -> ConcurrentMode #1171

Merged
merged 2 commits into from
Sep 27, 2018
Merged

AsyncMode -> ConcurrentMode #1171

merged 2 commits into from
Sep 27, 2018

Conversation

trueadm
Copy link
Contributor

@trueadm trueadm commented Sep 26, 2018

Follow up to facebook/react#13732. Renames AsyncMode to ConcurrentMode.

bvaughn
bvaughn suggested changes Sep 26, 2018
View reviewed changes
Copy link
Contributor

@bvaughn bvaughn left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This will break compat for all existing React releases that use AsyncMode – we'll need to support both.

So like, add the two new constants, and update the case statements so that async+concurrent mode both follow the same path, etc.

@trueadm
Copy link
Contributor Author

trueadm commented Sep 26, 2018

@bvaughn I thought that was the point here? It's an unstable API so can break in any version, right? I'm happy to add support in on the devtools, but the PR to rename AsyncMode has already landed in React – did you want me to revert it?

@bvaughn
Copy link
Contributor

bvaughn commented Sep 26, 2018

@bvaughn I thought that was the point here? It's an unstable API so can break in any version, right? I'm happy to add support in on the devtools, but the PR to rename AsyncMode has already landed in React – did you want me to revert it?

Well, yes– the API can change in that we can e.g. rename the export with a new release of react but we don't want a DevTools update to break compatibility with older releases. (DevTools tries to be compatible with all previous React versions.)

So in this case, we'll need the 16.x DevTools adapter to handle both AsyncMode and ConcurrentMode types. 😄

bvaughn
bvaughn reviewed Sep 26, 2018
View reviewed changes
backend/attachRendererFiber.js
ASYNC_MODE_SYMBOL_STRING: 'Symbol(react.async_mode)',
CONCURRENT_MODE_NUMBER: 0xeacf,
CONCURRENT_MODE_SYMBOL_STRING: 'Symbol(react.concurrent_mode)',
DEPRECATED_ASYNC_MODE_SYMBOL_STRING: 'Symbol(react.async_mode)',
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't sufficient. We need the number and string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The number is the same though?

Copy link
Contributor

Choose a reason for hiding this comment

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

Haha, d'oh! Ok sorry.

bvaughn
bvaughn approved these changes Sep 26, 2018
View reviewed changes
Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

I think this looks right 👍 Would be nice if you could give it a quick test with the existing release of React, as well as a build from master.

@quantizor
Copy link
Contributor

quantizor commented Sep 26, 2018 •
edited
Loading

It'd be cool if you reviewed my PR from 2 weeks ago that has its corresponding change already in react core and released. Thanks.

@trueadm trueadm merged commit 6f91f84 into facebook:master Sep 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bvaughn bvaughn bvaughn approved these changes

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@trueadm @bvaughn @quantizor @facebook-github-bot