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

Use useSyncExternalStore() #8785

Merged
merged 11 commits into from
Nov 16, 2021
Merged

Use useSyncExternalStore() #8785

merged 11 commits into from
Nov 16, 2021

Conversation

brainkim
Copy link
Contributor

@brainkim brainkim commented Sep 14, 2021

This PR reimplements useQuery()/useLazyQuery() with the proposed useSyncExternalStore() API, in preparation for React 18.

One note, the peer dependencies are likely to be all busted.

@brainkim brainkim force-pushed the brian-concurrent-mode-2 branch 5 times, most recently from be6660b to 053e5ca Compare September 20, 2021 23:26
@brainkim
Copy link
Contributor Author

This probably shouldn’t be merged until React 18 lands, but it’s also going to be a pain to have to deal with this both branch and the 3.5 branch at the same time.

@hwillson
Copy link
Member

@brainkim @benjamn should we go ahead and start a release-4.0 branch, and re-target this work against it? We're not going to be able to release this until useSyncExternalStore is publicly available, and even then we'll have to major version bump since we'll be updating our minimum react peer dep range.

@hwillson hwillson added this to the Release 3.6 milestone Oct 5, 2021
@hwillson hwillson added 2021-11 and removed 2021-10 labels Nov 1, 2021
@brainkim brainkim changed the base branch from release-3.5 to release-3.6 November 16, 2021 16:49
@brainkim brainkim marked this pull request as ready for review November 16, 2021 16:50
Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

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

Really awesome work @brainkim! 🎉

package.json Outdated
"optimism": "^0.16.1",
"prop-types": "^15.7.2",
"symbol-observable": "^4.0.0",
"ts-invariant": "^0.9.0",
"tslib": "^2.3.0",
"use-sync-external-store": "0.0.0-experimental-79b8fc667-20210920",
Copy link
Member

Choose a reason for hiding this comment

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

I think this can be bumped to 1.0.0-beta-4ff5f5719-20211115 now, or just beta.

Copy link
Contributor Author

@brainkim brainkim Nov 16, 2021

Choose a reason for hiding this comment

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

There are breaking changes ;_;

@hwillson
Copy link
Member

@brainkim as part of this PR we should probably consider adding react 18 to our supported peer dep range. We might also want to bump to react@beta in our dev deps for testing.

Copy link
Member

@benjamn benjamn left a comment

Choose a reason for hiding this comment

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

LGTM (at least for a v3.6 beta release)

@brainkim
Copy link
Contributor Author

squashing and merging here goes nothing

@brainkim brainkim merged commit 7f0d459 into release-3.6 Nov 16, 2021
@benjamn benjamn deleted the brian-concurrent-mode-2 branch November 16, 2021 23:09
@benjamn benjamn restored the brian-concurrent-mode-2 branch November 16, 2021 23:09
@brainkim brainkim deleted the brian-concurrent-mode-2 branch November 16, 2021 23:16
benjamn added a commit that referenced this pull request Nov 16, 2021
benjamn added a commit that referenced this pull request Feb 3, 2022
This reverts commit 7f0d459 and
follow-up commit 5328dae (move
onCompleted and onError to the snapshot function).

Because there have been several merges on release-3.6 since those
commits, and some of those merges involve useSyncExternalStore-related
code, I was not able to obtain a good state simply by reverting the
commits and manually resolving the conflicts.

Instead, I removed the commits during an interactive Git rebase, using
--rebase-merges to replay/recreate merge commits (rather than dropping
them, as rebase usually does). Once I verified tests were passing after
this rebase, I constructed the current commit from the difference
between the branches, which should allow us to keep the current history
from before the rebase, with this commit achieving the same final state
as after the rebase.

We will cherry-pick the inverse of this commit onto a new release-3.7
branch soon, so we can continue releasing v3.6 betas without the
risk/complexity introduced by useSyncExternalStore. We are still
committed to implementing that API and fully supporting React 18, with
prerelease availability continuing in the v3.7 betas.
benjamn added a commit that referenced this pull request Feb 3, 2022
This reverts commit 7f0d459 and
follow-up commit 5328dae (move
onCompleted and onError to the snapshot function).

Because there have been several merges on release-3.6 since those
commits, and some of those merges involve useSyncExternalStore-related
code, I was not able to obtain a good state simply by reverting the
commits and manually resolving the conflicts.

Instead, I removed the commits during an interactive Git rebase, using
--rebase-merges to replay/recreate merge commits (rather than dropping
them, as rebase usually does). Once I verified tests were passing after
this rebase, I constructed the current commit from the difference
between the branches, which should allow us to keep the current history
from before the rebase, with this commit achieving the same final state
as after the rebase.

We will cherry-pick the inverse of this commit onto a new release-3.7
branch soon, so we can continue releasing v3.6 betas without the
risk/complexity introduced by useSyncExternalStore. We are still
committed to implementing that API and fully supporting React 18, with
prerelease availability continuing in the v3.7 betas.
benjamn added a commit that referenced this pull request Feb 3, 2022
This reverts commit 7f0d459 (#8785)
and follow-up commit 5328dae (move
onCompleted and onError to the snapshot function).
@benjamn benjamn mentioned this pull request Feb 3, 2022
1 task
benjamn added a commit that referenced this pull request Feb 4, 2022
This reverts commit c6ab364, as
promised in PR #9393, fully restoring @brainkim's useSyncExternalStore
changes from PR #8785.
@wintercounter
Copy link

Can't this be released earlier using https://www.npmjs.com/package/use-sync-external-store ? It's an official, backward compatible shim, so it can work with < 18.

@benjamn
Copy link
Member

benjamn commented Mar 23, 2022

Quite possibly @wintercounter! We held off before because using the shim required substantial refactoring, but thanks to #9459 I believe that refactoring may not be as complicated/risky now. Still aiming to release v3.6 soon, and restore useSyncExternalStore on the release-3.7 branch (also soon).

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants