-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
React refactorings #8596
React refactorings #8596
Conversation
e06a3f2
to
54f46cb
Compare
e758832
to
2f28e31
Compare
4b98507
to
a7f60ba
Compare
cd10c4d
to
95422e1
Compare
e7fc323
to
9d33727
Compare
3eaa6c1
to
fbf42b6
Compare
732bd3c
to
c2fa63f
Compare
fbf42b6
to
d583f4f
Compare
ef9c30a
to
baeb69c
Compare
3362d44
to
790d7f8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really exciting!
Could you rebase against the release-3.5
branch, so we can put out a beta release with these changes?
setResult({ | ||
loading: false, | ||
data: void 0, | ||
error: void 0, | ||
variables: options?.variables, | ||
}); | ||
setObservable(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these state updates automatically batched because they're in a useEffect
callback?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m not sure, but my guess is no. There are a lot of subtle timing changes which this PR inevitably produces as we refactor state calls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brainkim Maybe let's use unstable_batchedUpdates
then, just in case? Or are you saying it's important to preserve the timing of the separate updates?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@benjamn
It is impossible to preserve the exact timings of the old React hooks implementation, insofar as it used a useReducer/forceUpdate
pattern, which, among other things, would in many circumstances, cause state changes to be happen directly in the render function, which is apparently a bad thing to do (we still do it in useQuery
just so some specific edge cases don’t produce confusing unit test results). And I’m still not sure what the effect of “batching” the two state updates would have, given that the result and observable are two independently updating pieces of state.
I’m not familiar with unstable_batchedUpdates
. Have you adopted it somewhere in this codebase?
- What is availability of
unstable_batchedUpdates
across React versions? - Is it okay that it seems to be defined on
ReactDOM
, when none of the hooks require theReactDOM
package previously, especially given that some of our poor users are using React Native? - Is it a merely an optimization, or does it actually affect the logic of the hook?
- Would it hamper preact support?
if (__DEV__) { | ||
// ensure we run an update after refreshing so that we can resubscribe | ||
useAfterFastRefresh(forceUpdate); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we losing this functionality, or is it no longer needed after the refactoring?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m not sure; I’ll have to look into it. We don’t call side-effects from the render function for the most part anymore, except for certain edge cases where I could not get the tests working.
87f00e9
to
1933ff1
Compare
410e1ca
to
73569ef
Compare
8c4b091
to
cf90a3d
Compare
NEVER MIND I’M PUTTING IT IN A SEPARATE PR. |
cf90a3d
to
7b95b24
Compare
We temporarily relaxed the bundlesize limit to include only `@apollo/client/core` during development of Apollo Client v3.4, in part because `QueryData` and other React-related infrastructure were unnecessarily verbose, as you can see from the jump in bundlesize from 24.7kB to 28.4kB because of this commit. However, @brainkim refactored the React abstractions in #8596 🎉, eliminating `QueryData` and related classes once and for all, so I now feel much better about including `@apollo/client/react` in the bundlesize calculation again.
Analogous to 2c0088f, but cherry-picked from `main` to `release-3.5`. Though this increases the bundlesize by +2.7kB (reflecting the weight of `@apollo/client/react`), the same calculation on `main` gives 28.35kB, whereas `release-3.5` now gives 27.62kB, reflecting the massive savings from @brainkim's React refactorings in #8596.
This PR eliminates the XData class-based abstractions which were used to implement the React hooks, in favor of hooks which are more “idiomatic” to React. The old class-based abstractions generated an immense amount of mental overhead, insofar as it attempted to abstract over React hooks by defining two methods
execute()
andafterExecute()
which are called by the hook body and in auseEffect()
hook respectively.This PR should make the following issues easier to fix:
onCompleted
is broken in v3.0.0 release (CodeSandbox repro) #6636onCompleted
gets called after loading state has been changed.. #8350