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

Imperative read and write methods #1310

Merged
merged 36 commits into from
Mar 1, 2017
Merged
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
03f7f76
add read method to ApolloClient
calebmer Feb 9, 2017
f5f2ba6
add tests for ApolloClient
calebmer Feb 9, 2017
c1acb42
rename test variable
calebmer Feb 10, 2017
f496df2
move assertion
calebmer Feb 10, 2017
d246ca5
split read into readQuery and readFragment
calebmer Feb 10, 2017
bce014a
add variables to readFragment
calebmer Feb 13, 2017
fec7003
style
calebmer Feb 17, 2017
33d60fe
add imperative write methods to client
calebmer Feb 17, 2017
edfadae
add optimistic writes
calebmer Feb 18, 2017
bd6a00f
add changelog entry
calebmer Feb 18, 2017
2bc3457
Merge branch 'master' into feat/imperative-read-write
calebmer Feb 18, 2017
baff038
Update CHANGELOG.md
calebmer Feb 22, 2017
37cd7b9
Update ApolloClient.ts
calebmer Feb 22, 2017
b716e19
return null when data from an id cannot be found
calebmer Feb 22, 2017
793c59d
reword docs and add argument docs
calebmer Feb 22, 2017
116f9b0
swap “state” for “store” in test names
calebmer Feb 22, 2017
23eb614
move error tests to suite top
calebmer Feb 22, 2017
d3b3995
add an error for operations in getFragmentQuery
calebmer Feb 22, 2017
dc915ec
add tests for getFragmentQueryDocument
calebmer Feb 22, 2017
f56d476
add a data proxy and transactions
calebmer Feb 24, 2017
aa182fa
Merge branch 'master' into feat/imperative-read-write
helfer Feb 25, 2017
72ad7c6
remove write transaction from client
calebmer Feb 27, 2017
b2b9ae7
add tests for proxies
calebmer Feb 27, 2017
2e965a2
add mutation update function
calebmer Feb 27, 2017
3f57b27
add mutation update function tests
calebmer Feb 27, 2017
238f883
Merge branch 'feat/imperative-read-write' of github.com:apollostack/a…
calebmer Feb 27, 2017
312f96a
fix proxy tests
calebmer Feb 27, 2017
e108dde
Merge branch 'master' into feat/imperative-read-write
calebmer Feb 27, 2017
e69c858
update changelog
calebmer Feb 27, 2017
c42b0c2
remove write(Query|Fragment)Optimistically
calebmer Feb 27, 2017
3c7f401
change internal documentation phrasing
calebmer Feb 28, 2017
f2a6fc7
make data proxy arguments named arguments
calebmer Feb 28, 2017
97686e3
apply writes locally in transactions
calebmer Feb 28, 2017
4103831
Merge branch 'master' into feat/imperative-read-write
calebmer Feb 28, 2017
c6335a6
Merge branch 'master' into feat/imperative-read-write
helfer Mar 1, 2017
d11a4b4
Merge branch 'master' into feat/imperative-read-write
helfer Mar 1, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ Expect active development and potentially significant breaking changes in the `0

### vNEXT
- Prefer stale data over partial data in cases where a user would previously get an error. [PR #1306](https://github.com/apollographql/apollo-client/pull/1306)
- Add direct cache manipulation read and write methods to provide the user the power to interact with Apollo’s GraphQL data representation outside of mutations. [PR #1310](https://github.com/apollographql/apollo-client/pull/1310)
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to rebase the changelog

- Update TypeScript `MutationOptions` definition with the new object type available in `refetchQueries`. [PR #1315](https://github.com/apollographql/apollo-client/pull/1315)
- Add `fetchMore` network status to enable loading information for `fetchMore` queries. [PR #1305](https://github.com/apollographql/apollo-client/pull/1305)
- ...
Expand Down
245 changes: 245 additions & 0 deletions src/ApolloClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import {
SelectionSetNode,
/* tslint:enable */

DocumentNode,
FragmentDefinitionNode,
} from 'graphql';

import {
Expand Down Expand Up @@ -58,6 +60,16 @@ import {
storeKeyNameFromFieldNameAndArgs,
} from './data/storeUtils';

import {
getFragmentQueryDocument,
} from './queries/getFromAST';

import {
DataProxy,
ReduxDataProxy,
TransactionDataProxy,
} from './data/proxy';

import {
version,
} from './version';
Expand Down Expand Up @@ -100,6 +112,8 @@ export default class ApolloClient {
public queryDeduplication: boolean;

private devToolsHookCb: Function;
private optimisticWriteId: number;
private proxy: DataProxy | undefined;

/**
* Constructs an instance of {@link ApolloClient}.
Expand Down Expand Up @@ -232,6 +246,8 @@ export default class ApolloClient {
}

this.version = version;

this.optimisticWriteId = 1;
}

/**
Expand Down Expand Up @@ -336,6 +352,219 @@ export default class ApolloClient {
return this.queryManager.startGraphQLSubscription(realOptions);
}

/**
* Tries to read some data from the store in the shape of the provided
* GraphQL query without making a network request. This method will start at
* the root query. To start at a specific id returned by `dataIdFromObject`
* use `readFragment`.
*
* @param query The GraphQL query shape to be used.
*
* @param variables Any variables that the GraphQL query may depend on.
*/
public readQuery<QueryType>(
query: DocumentNode,
variables?: Object,
): QueryType {
return this.initProxy().readQuery<QueryType>(query, variables);
}

/**
* Tries to read some data from the store in the shape of the provided
* GraphQL fragment without making a network request. This method will read a
* GraphQL fragment from any arbitrary id that is currently cached, unlike
* `readQuery` which will only read from the root query.
*
* You must pass in a GraphQL document with a single fragment or a document
* with multiple fragments that represent what you are reading. If you pass
* in a document with multiple fragments then you must also specify a
* `fragmentName`.
*
* @param id The root id to be used. This id should take the same form as the
* value returned by your `dataIdFromObject` function. If a value with your
* id does not exist in the store, `null` will be returned.
*
* @param fragment A GraphQL document with one or more fragments the shape of
* which will be used. If you provide more then one fragments then you must
* also specify the next argument, `fragmentName`, to select a single
* fragment to use when reading.
*
* @param fragmentName The name of the fragment in your GraphQL document to
* be used. Pass `undefined` if there is only one fragment and you want to
* use that.
*
* @param variables Any variables that your GraphQL fragments depend on.
*/
public readFragment<FragmentType>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Just out of curiosity: what error gets thrown if you provide an id that doesn't exist in the store? I have a hunch that we might want to improve that error message for this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. It would return an empty object because the fragment pattern matching wouldn’t match anything. I made it return null instead and added a test 👍

id: string,
fragment: DocumentNode,
fragmentName?: string,
variables?: Object,
): FragmentType | null {
return this.initProxy().readFragment<FragmentType>(id, fragment, fragmentName, variables);
}

/**
* Writes some data in the shape of the provided GraphQL query directly to
* the store. This method will start at the root query. To start at a a
* specific id returned by `dataIdFromObject` then use `writeFragment`.
Copy link
Contributor

Choose a reason for hiding this comment

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

There's an extra "a" and "then".

*
* @param data The data you will be writing to the store.
*
* @param query The GraphQL query shape to be used.
*
* @param variables Any variables that the GraphQL query may depend on.
*/
public writeQuery(
data: any,
query: DocumentNode,
variables?: Object,
): void {
return this.initProxy().writeQuery(data, query, variables);
}

/**
* Writes some data in the shape of the provided GraphQL fragment directly to
* the store. This method will write to a GraphQL fragment from any arbitrary
* id that is currently cached, unlike `writeQuery` which will only write
* from the root query.
*
* You must pass in a GraphQL document with a single fragment or a document
* with multiple fragments that represent what you are writing. If you pass
* in a document with multiple fragments then you must also specify a
* `fragmentName`.
*
* @param data The data you will be writing to the store.
*
* @param id The root id to be used. This id should take the same form as the
* value returned by your `dataIdFromObject` function.
*
* @param fragment A GraphQL document with one or more fragments the shape of
* which will be used. If you provide more then one fragments then you must
* also specify the next argument, `fragmentName`, to select a single
* fragment to use when reading.
*
* @param fragmentName The name of the fragment in your GraphQL document to
* be used. Pass `undefined` if there is only one fragment and you want to
* use that.
*
* @param variables Any variables that your GraphQL fragments depend on.
*/
public writeFragment(
Copy link
Contributor

Choose a reason for hiding this comment

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

How do we feel about making this take keyword arguments instead of 5 positional arguments? Long lists of positional arguments are fine in TS/Flow but I find them very hard to use in JS.

Copy link
Contributor Author

@calebmer calebmer Feb 27, 2017

Choose a reason for hiding this comment

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

They were named at first, I just really like the look of:

client.readQuery(gql`{ ... }`);
client.writeQuery({ ... }, gql`{ ... }`);

vs.

client.readQuery({
  query: gql`{ ... }`,
});
client.writeQuery({
  data: { ... },
  query: gql`{ ... }`,
});

and so I decided to make everything positional instead of named. I don’t actually think it is that bad with writeFragment being the longest. As follows is the most common case and the longest case for both readFragment and writeFragment:

// Common

client.readFragment(
  'Todo42',
  gql`fragment todo on Todo { text, completed }`,
);

client.writeFragment(
  { text: 'Clean up', completed: false },
  'Todo42',
  gql`fragment todo on Todo { text, completed }`,
);

// Uncommon

client.readFragment(
  'Todo42',
  gql`
    fragment todo1 on Todo { ...todo2 @include(if: $isTrue) }
    fragment todo2 on Todo { text, completed }
  `,
  'todo1',
  { isTrue: true },
);

client.writeFragment(
  { text: 'Clean up', completed: false },
  'Todo42',
  gql`
    fragment todo1 on Todo { ...todo2 @include(if: $isTrue) }
    fragment todo2 on Todo { text, completed }
  `,
  'todo1',
  { isTrue: true },
);

Most of the time you will only have one fragment and therefore won’t need the fragmentName, and also you would rarely have variables.

I understand the desire for named arguments. I just really like how the 80% case looks with positional arguments 😊. Let me know if you still think named arguments are worth it.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can always change it later.

However, looking at that one example, can we reorder the args to always have the same order?

  1. ID (required)
  2. Fragment (required)
  3. Data (required for write)
  4. Variables (used sometimes)
  5. Fragment name (used very rarely IMO)

I think there are a few heuristics for ordering:

  1. How commonly will it be used? Required args should go first.
  2. How long is the argument? In JS I think it looks nicer to have a small argument, like an ID, be in the first position.

However in some sense the fact that we have to think about ordering is a bit unfortunate. At the end of the day it doesn't matter that much, but this seems like the kind of API I would need to look up in the docs frequently to remember how to call it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer named arguments even if it makes the 80% use-case more verbose, because it makes the code easier to read. Rather than having to guess what each arg represents, you know right away because it's named.

data: any,
id: string,
fragment: DocumentNode,
fragmentName?: string,
variables?: Object,
): void {
return this.initProxy().writeFragment(data, id, fragment, fragmentName, variables);
}

/**
* Writes some data in the shape of the provided GraphQL query directly to
* the store. This method will start at the root query. To start at a
* specific id returned by `dataIdFromObject` then use
* `writeFragmentOptimistically`.
*
* Unlike `writeQuery`, the data written with this method will be stored in
* the optimistic portion of the cache and so will not be persisted. This
* optimistic write may also be rolled back with the `rollback` function that
* was returned.
*
* @param data The data you will be writing to the store.
*
* @param query The GraphQL query shape to be used.
*
* @param variables Any variables that the GraphQL query may depend on.
*/
public writeQueryOptimistically(
data: any,
query: DocumentNode,
variables?: Object,
): {
rollback: () => void,
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll have to explain in the docs why there's no commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no reason why we could not add a commit. We would just rollback and call: writeQuery 😊

It wouldn’t be efficient, but it would work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, but I'd rather just tell people to do that themselves.

} {
const optimisticWriteId = (this.optimisticWriteId++).toString();
this.initStore();
this.store.dispatch({
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these implemented via a direct dispatch, while the others use the proxy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Two reasons:

  1. write(Query|Fragment)Optimistically are not included in the proxy. This is intentional because a proxy may implicitly write optimistically depending on the context it is in.
  2. this.optimisticWriteId lives on ApolloClient to generate globally unique ids. This is something we would need to feed into a proxy.

However, now that you mention it, removing write(Query|Fragment)Optimistically would actually remove a bit of complexity from this PR. They were originally added before we had the update function on mutate, however that now serves as a much better way to manage optimistic writes.

I don’t think anyone would miss these methods if we removed them, and it would reduce complexity. I’m going to go ahead and remove these two methods. If you think there is a case for them we can add them back 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds great to me! So what you're saying is, people can still use optimistic writes from the update callback in a mutation, but there will be no way to do it in a totally standalone way. That works for me for a first pass.

The only reason you would need optimistic writes otherwise is if you were using REST to do mutations and wanted to do an optimistic update to your store before refetching or something, but that is a somewhat niche thing and can be added later as a feature.

type: 'APOLLO_WRITE_OPTIMISTIC',
optimisticWriteId,
writes: [{
rootId: 'ROOT_QUERY',
result: data,
document: query,
variables: variables || {},
}],
});
return {
rollback: () => this.store.dispatch({
type: 'APOLLO_WRITE_OPTIMISTIC_ROLLBACK',
optimisticWriteId,
}),
};
}

/**
* Writes some data in the shape of the provided GraphQL fragment directly to
* the store. This method will write to a GraphQL fragment from any
* arbitrary id that is currently cached, unlike `writeQueryOptimistically`
* which will only write from the root query.
*
* You must pass in a GraphQL document with a single fragment or a document
* with multiple fragments that represent what you are writing. If you pass
* in a document with multiple fragments then you must also specify a
* `fragmentName`.
*
* Unlike `writeFragment`, the data written with this method will be stored in
* the optimistic portion of the cache and so will not be persisted. This
* optimistic write may also be rolled back with the `rollback` function that
* was returned.
*
* @param data The data you will be writing to the store.
*
* @param id The root id to be used. This id should take the same form as the
* value returned by your `dataIdFromObject` function.
*
* @param fragment A GraphQL document with one or more fragments the shape of
* which will be used. If you provide more then one fragments then you must
* also specify the next argument, `fragmentName`, to select a single
* fragment to use when reading.
*
* @param fragmentName The name of the fragment in your GraphQL document to
* be used. Pass `undefined` if there is only one fragment and you want to
* use that.
*
* @param variables Any variables that your GraphQL fragments depend on.
*/
public writeFragmentOptimistically(
data: any,
id: string,
fragment: DocumentNode,
fragmentName?: string,
variables?: Object,
): {
rollback: () => void,
} {
const optimisticWriteId = (this.optimisticWriteId++).toString();
this.initStore();
this.store.dispatch({
type: 'APOLLO_WRITE_OPTIMISTIC',
optimisticWriteId,
writes: [{
rootId: id,
result: data,
document: getFragmentQueryDocument(fragment, fragmentName),
variables: variables || {},
}],
});
return {
rollback: () => this.store.dispatch({
type: 'APOLLO_WRITE_OPTIMISTIC_ROLLBACK',
optimisticWriteId,
}),
};
}

/**
* Returns a reducer function configured according to the `reducerConfig` instance variable.
*/
Expand Down Expand Up @@ -457,4 +686,20 @@ export default class ApolloClient {
queryDeduplication: this.queryDeduplication,
});
};

/**
* Initializes a data proxy for this client instance if one does not already
* exist and returns either a previously initialized proxy instance or the
* newly initialized instance.
*/
private initProxy(): DataProxy {
if (!this.proxy) {
this.initStore();
this.proxy = new ReduxDataProxy(
this.store,
this.reduxRootSelector || defaultReduxRootSelector,
);
}
return this.proxy;
}
}
50 changes: 47 additions & 3 deletions src/actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ import {
MutationQueryReducer,
} from './data/mutationResults';

import {
DataProxy,
} from './data/proxy';

import {
ApolloReducer,
} from './store';
Expand Down Expand Up @@ -89,6 +93,7 @@ export interface MutationInitAction {
optimisticResponse: Object | undefined;
extraReducers?: ApolloReducer[];
updateQueries?: { [queryId: string]: MutationQueryReducer };
update?: (proxy: DataProxy, mutationResult: Object) => void;
}

export function isMutationInitAction(action: ApolloAction): action is MutationInitAction {
Expand All @@ -105,6 +110,7 @@ export interface MutationResultAction {
mutationId: string;
extraReducers?: ApolloReducer[];
updateQueries?: { [queryId: string]: MutationQueryReducer };
update?: (proxy: DataProxy, mutationResult: Object) => void;
}

export function isMutationResultAction(action: ApolloAction): action is MutationResultAction {
Expand Down Expand Up @@ -141,20 +147,55 @@ export function isStoreResetAction(action: ApolloAction): action is StoreResetAc
return action.type === 'APOLLO_STORE_RESET';
}

export type SubscriptionResultAction = {
export interface SubscriptionResultAction {
type: 'APOLLO_SUBSCRIPTION_RESULT';
result: ExecutionResult;
subscriptionId: number;
variables: Object;
document: DocumentNode;
operationName: string;
extraReducers?: ApolloReducer[];
};
}

export function isSubscriptionResultAction(action: ApolloAction): action is SubscriptionResultAction {
return action.type === 'APOLLO_SUBSCRIPTION_RESULT';
}

export interface DataWrite {
rootId: string;
result: any;
document: DocumentNode;
variables: Object;
}

export interface WriteAction {
type: 'APOLLO_WRITE';
writes: Array<DataWrite>;
}

export function isWriteAction(action: ApolloAction): action is WriteAction {
return action.type === 'APOLLO_WRITE';
}

export interface WriteActionOptimistic {
type: 'APOLLO_WRITE_OPTIMISTIC';
optimisticWriteId: string;
writes: Array<DataWrite>;
}

export function isWriteOptimisticAction(action: ApolloAction): action is WriteActionOptimistic {
return action.type === 'APOLLO_WRITE_OPTIMISTIC';
}

export interface WriteActionOptimisticRollback {
type: 'APOLLO_WRITE_OPTIMISTIC_ROLLBACK';
optimisticWriteId: string;
}

export function isWriteOptimisticRollbackAction(action: ApolloAction): action is WriteActionOptimisticRollback {
return action.type === 'APOLLO_WRITE_OPTIMISTIC_ROLLBACK';
}

export type ApolloAction =
QueryResultAction |
QueryErrorAction |
Expand All @@ -166,4 +207,7 @@ export type ApolloAction =
MutationErrorAction |
UpdateQueryResultAction |
StoreResetAction |
SubscriptionResultAction;
SubscriptionResultAction |
WriteAction |
WriteActionOptimistic |
WriteActionOptimisticRollback;
Loading