-
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
[WIP] Add client side support for @defer directive #3686
Conversation
f92550b
to
1c08016
Compare
0eb55e0
to
bc2ef68
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 looking AMAZING @clarencenpy! See my comments inline (nothing major). Thanks!
// If loadingState is present, this is a patch from a deferred | ||
// query, and we should always treat it as a different result | ||
// even though the actual data might be the same (i.e. the patch's | ||
// data could be 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.
Missing closing ")" in comment.
* Recursive function that extracts a tree that mirrors the shape of the query, | ||
* adding _loading property to fields which are deferred. | ||
*/ | ||
function extractDeferredFieldsToTree( |
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.
Regarding extractDeferredFieldsToTree
and initDeferredFieldLoadingStates
, I see they're defined as utility functions outside of the QueryManager
class. I don't really have any issues with this, but since the rest of the QueryManager
functionality is already defined in the class, would you mind moving these into QueryManager
as private
functions?
* Only a partial response has been received, this could come from the usage | ||
* of @defer/live/stream directives. | ||
*/ | ||
partial = 9, |
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 sooooo exciting! 🎉
@@ -52,5 +58,5 @@ export enum NetworkStatus { | |||
export function isNetworkRequestInFlight( | |||
networkStatus: NetworkStatus, | |||
): boolean { | |||
return networkStatus < 7; | |||
return networkStatus < 7 || networkStatus === 9; |
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 going to pose a bit of an interesting problem. I've seen a lot of 3rd party code that is using the networkStatus < 7
check to see if a request is in flight. It's too bad the NetworkStatus
enum wasn't originally defined using increments of 10, so the partial status could have been added just after poll
(so poll
at 60, partial
at 61). That being said, progress is progress - there isn't much we can do here for now, so 👍.
|
||
/** | ||
* Define a new type for patches that are sent as a result of using defer. | ||
* Its is basically the same as ExecutionResult, except that it has a "path" |
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.
Small grammar issue - should be "It is".
* field that keeps track of the where the patch is to be merged with the | ||
* original result. | ||
*/ | ||
export interface ExecutionPatchResult { |
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.
It looks like ExecutionPatchResult
is very similar to ExecutionResult
(just adds path
). You might be able to extend
the ExecutionResult
interface instead?
@@ -11,6 +13,8 @@ export type QueryStoreValue = { | |||
networkStatus: NetworkStatus; | |||
networkError?: Error | null; | |||
graphQLErrors?: GraphQLError[]; | |||
_loadingState?: Record<string, any>; | |||
loadingState?: Record<string, any>; |
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.
Could we avoid the underscore naming here - seeing both _loadingState
and loadingState
is a bit confusing. Maybe _loadingState
should be called something like originalLoadingState
?
* The structure of this will mirror the response data, with deferred fields | ||
* set to undefined until its patch is received. | ||
*/ | ||
function compactLoadingStateTree( |
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.
Same comment as earlier - would you mind moving this into the QueryStore
class, as a private
function?
patch: ExecutionPatchResult, | ||
): void { | ||
if (patch.errors) { | ||
} |
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 assuming this is because these changes are in alpha state, but just calling this out as a reminder. Something needs to be done with patch.errors
.
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.
The logic to do that is in queries.ts
, where I merge errors for each patch into the graphQLErrors
array. My bad, forgot to take this out.
The existing page for "Add React to a New App" (previously; now broken: https://reactjs.org/docs/add-react-to-a-new-app.html) has been moved to https://reactjs.org/docs/create-a-new-react-app.html. This updates the documentation to accommodate that change!
The motivation behind this is the need to distinguish between "pending" or "null" states for deferred fields.
This allows us to omit `children` from the access path.
Even though the actual data might look the same (i.e. the patched data is null)
@defer must be specified for all of the selections, matching up with the behavior of Apollo Server.
# Conflicts: # docs/source/features/defer-support.md # packages/apollo-client/package.json # packages/apollo-client/src/core/QueryManager.ts
Generated by 🚫 dangerJS |
👋 Hi! Is this something that you all are still working on? I'd love to use |
some update for this feature? 🤓 |
Defer will add so much value... Anyone working on this? 1 year later... what's blocking this? :-) |
Fully integrated production |
@hwillson Any rough approximation of the featuring landing? 2019 or 2020? |
@lukejagodzinski I think it's safe to say 2020 as this point. |
@hwillson thanks, but early 2020, mid, or late? :D It's also a little bit misleading to watch some videos about @defer on YT, see it in some places in docs, read article about it from 2018 and still not being able to test even experimental version :). I'm just saying, that after seeing so many old news about that, I assumed that this feature was already implemented. Until I digged deeper :). Maybe, it could be mentioned in the docs that this feature is still WIP? Just create a new section in docs, so anyone who will encounter news about @defer and go to the docs, will see that it's not implemented yet :) |
Thanks @lukejagodzinski - great point. We had a defer section in the docs, but we removed it when work on As for when AS 3 is launching, I just asked the server team and they told me ... 2020 🙂. For more specific details around AS 3 and |
@hwillson ok thanks! :) Can't wait for it :) Keep doing great work! |
Any updates here? Looks like there hasn't been any activity for a year neither here nor in the Apollo Server repo |
@fabis94 i hope that graphql/graphql-js#2839 is the next step for this PR |
@KatFishSnake this work is now being tracked in #8184, and will be part of Apollo Client 4. |
Changes:
loadingState
networkStatus
for deferred queries that are still streamingThis depends on an alpha release of apollo-link-dedup #714.