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

Make Operation Context serializable #111

Closed
smkhalsa opened this issue May 18, 2020 · 6 comments
Closed

Make Operation Context serializable #111

smkhalsa opened this issue May 18, 2020 · 6 comments

Comments

@smkhalsa
Copy link
Member

I mentioned the issue of serializing Context in #110, but I figured I'd open a separate issue to discuss this further.

As far as I can tell, some way of serializing data passed into Context is necessary to enable first-class support for offline-first use cases.

For example, let's say we have a mutation that requires passing the userId as an argument. We might create a ContextEntry such as the following:

class UserContextEntry extends ContextEntry {
  final String userId;

  UserContextEntry(this.userId);

  @override
  List<Object> get fieldsForEquality => [userId];
}

If the app is offline, a GraphQL client will need to persist this data along with the query in order to properly execute the mutation when the app comes back online.

We could potentially work around this using code generators without adjusting the existing Context implementation. For example, we could create an aggregate builder that searches source code for implementations of ContextEntry, then builds a serializer that maps type names to types so that ContextEntrys can be deserialized later. This would, of course, require each ContextEntry to be serializable, meaning that our UserContextEntry above would probably need to be extended to something like this:

class UserContextEntry extends ContextEntry implements SerializableContextEntry {
  final String userId;

  UserContextEntry(this.userId);

  @override
  List<Object> get fieldsForEquality => [userId];

  Map<String, dynamic> toJson() => {"userId": userId};

  UserContextEntry fromJson(Map<String, dynamic> json) =>
      UserContextEntry(json["userId"]);
}

This is starting to be a lot of boilerplate just to pass a userId to a mutation.

@klavs @micimize I'd appreciate any insight you may have on this.

@smkhalsa smkhalsa changed the title Make Operation Context executable Make Operation Context serializable May 18, 2020
@micimize
Copy link
Contributor

Shouldn't we be able to store the raw already-serialized request that will be sent over the wire?

@smkhalsa
Copy link
Member Author

@micimize I believe we'd need the deserialized request to pass into any cache update functions.

@micimize
Copy link
Contributor

hmm... I would think cache updates should depend on responses. So conceptually, the cache should update in response to an optimistic response, which has optimistic data and context.

Response contexts only depend on the execution result:

Stream<Response> request(
Request request, [
NextLink forward,
]) async* {
final httpResponse = await _executeRequest(
_getHttpLinkHeaders(request),
_serializeRequest(request),
);
final response = _parseHttpResponse(httpResponse);
if (httpResponse.statusCode >= 300 ||
(response.data == null && response.errors == null)) {
throw HttpLinkServerException(
response: httpResponse,
parsedResponse: response,
);
}
yield Response(
data: response.data,
errors: response.errors,
context: _updateResponseContext(response, httpResponse),
);
}

@smkhalsa
Copy link
Member Author

If optimistic data is provided, the update function should get called twice: first with the optimistic data, then again with the actual response (with the optimistic data and updates discarded).

Of course, if no optimistic data is provided, update is only run with the actual response.

Either way, if the client is offline when the mutation is made, then the app comes online, we'd have to execute the query, then run the cache update function with the result.

@klavs
Copy link
Contributor

klavs commented May 21, 2020

Here's my comment from #110:

Context may have both useful and useless information. (Or critical and nice-to-have information). I tend to think that Context should not be serializable because it kind-of contains the current state of request execution. It might be a fundamental design flaw of the Context if it's allowed to be used as a container for internal state and a container external request parameters. Maybe it should be hidden from the users of a graphql client, by providing an alternative approach which lets the client decide what's serializable and what's not.

@smkhalsa
Copy link
Member Author

I'm closing this as I decided to use a separate JSON object for cache handler state rather than using Context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants