Add imperative read method to ApolloClient

[calebmer]

I told you it was pretty easy @helfer 😊

The major thing this adds is the ability to start at any id from readQueryFromStore. There are now tests for that functionality.

I did not add tests for the method on ApolloClient, yet, because it does not look like there is a test file for the ApolloClient class at the moment. I assume we will want one though when we add write as well.

• Make sure all of the significant new logic is covered by tests
• Rebase your changes on master so that they can be merged easily
• Make sure all tests and linter rules pass
• Update CHANGELOG.md with your change
• If this was a change that affects the external API, update the docs and post a link to the PR in the discussion

[calebmer]

Usage for this function would look like:

client.read({
  selection: gql`{ viewer { name } }`,
});

When reading from the root, or:

client.read({
  selection: gql`{ name }`,
  id: client.dataId(viewer),
});

When reading from a given id.

An alternative API would be:

client.read(gql`{ viewer { name } }`);
client.read(gql`{ name }`, client.dataId(viewer));
client.read(gql`{ name }`, 'User-5');

However, this would make it harder to pass in the returnPartialData option and variables option. variables may always be the third argument, or expressed directly in the query as literals, but then we are left with returnPartialData. In that scenario, we should either always set returnPartialData to true or false (probably true, though). In the future when we have no partial data at all this won’t be an issue.

The latter API is definitely more beautiful, so we have to ask if we want to make these compromises to get the more beautiful API.

[helfer]

Awesome! I'll take a look at this first thing tomorrow morning before I start implementing the write method 🙂

[calebmer]

Also, what’s the procedure for updating the documentation? 😊

We may want to update the docs with both read and write at once, but that’s something I could start writing.

[helfer]

@calebmer There's no fancy procedure. You just pick the place where you think it should go, and start writing. If necessary, you edit the config file as well. This can probably go into the core-docs at first, but once we have the onResult (the thing that runs after mutations, queries, subscriptions), we should also document that for React and Angular.

[helfer]

Looks great! Just a few nitpicks about the last test.

[helfer]

This is pretty hard to read, why not just write out the store like in other tests?

[calebmer]

This was just a copy paste of this test:

apollo-client/test/readFromStore.ts

Lines 155 to 246 in 5e944c1

	it('runs a nested query with multiple fragments', () => {
	const result: any = {
	id: 'abcd',
	stringField: 'This is a string!',
	numberField: 5,
	nullField: null,
	nestedObj: {
	id: 'abcde',
	stringField: 'This is a string too!',
	numberField: 6,
	nullField: null,
	} as StoreObject,
	deepNestedObj: {
	stringField: 'This is a deep string',
	numberField: 7,
	nullField: null,
	} as StoreObject,
	nullObject: null,
	__typename: 'Item',
	};
	const store = {
	'ROOT_QUERY': assign({}, assign({}, omit(result, 'nestedObj', 'deepNestedObj')), {
	__typename: 'Query',
	nestedObj: {
	type: 'id',
	id: 'abcde',
	generated: false,
	},
	}) as StoreObject,
	abcde: assign({}, result.nestedObj, {
	deepNestedObj: {
	type: 'id',
	id: 'abcdef',
	generated: false,
	},
	}) as StoreObject,
	abcdef: result.deepNestedObj as StoreObject,
	} as NormalizedCache;
	const queryResult = readQueryFromStore({
	store,
	query: gql`
	{
	stringField,
	numberField,
	nullField,
	... on Query {
	nestedObj {
	stringField
	nullField
	deepNestedObj {
	stringField
	nullField
	}
	}
	}
	... on Query {
	nestedObj {
	numberField
	nullField
	deepNestedObj {
	numberField
	nullField
	}
	}
	}
	... on Query {
	nullObject
	}
	}
	`,
	});
	// The result of the query shouldn't contain __data_id fields
	assert.deepEqual(queryResult, {
	stringField: 'This is a string!',
	numberField: 5,
	nullField: null,
	nestedObj: {
	stringField: 'This is a string too!',
	numberField: 6,
	nullField: null,
	deepNestedObj: {
	stringField: 'This is a deep string',
	numberField: 7,
	nullField: null,
	},
	},
	nullObject: null,
	});
	});

I don’t like it either, but it’s consistent.

[helfer]

result might not be the best name here. Something like storeObjects maybe?

[calebmer]

Renamed it data. I don’t like it either, but this test is basically just a copy paste of another test in this file 😉

[helfer]

I would put this right after you run the query, its' a bit easier to read that way.

[calebmer]

Sure. At some point I had a reason for doing it this way, but I can’t remember anymore.

[stubailo]

I think we should have reads that start with an ID use a fragment, because otherwise the query won't validate if you are using something like eslint-plugin-graphql.

As a bonus, if you use a fragment and specify the type, that will validate that you are querying fields that exist.

[helfer]

I agree that we should expect an operation definition when no id is provided.

However, when an id is provided and we expect a fragment, we'll have to enforce a convention for which fragment is to be used as the first selection set (because the fragment may itself contain fragments). Is it clear enough if we just always take the first fragment? We could also allow only a single fragment, but I think that would be too restrictive.

An alternative would be to introduce a special cacheNode type, which just selects that particular cache node.

I think the cacheNode option is cleaner and clearer from the user's perspective, but it introduces special cases in apollo-client and eslint-plugin-graphql, so I'm not 100% sure if that's what we should go with.

[calebmer]

I think we should have reads that start with an ID use a fragment, because otherwise the query won't validate if you are using something like eslint-plugin-graphql.

As a bonus, if you use a fragment and specify the type, that will validate that you are querying fields that exist.

@stubailo For the fragment case I expected people to do something like:

client.read(gql`
  { ...userFragment }

  fragment userFragment on User {
    name
  }
`)

…or use template interpolation:

client.read(gql`
  { ...userFragment }

  ${USER_FRAGMENT}
`)

I was not thinking about the eslint-plugin-graphql case, however. Wouldn’t the above two examples pass eslint-plugin-graphql? Since I assume eslint-plugin-graphql would have a tough time getting the type of a fragment.

I’m not against passing fragments directly, my one concern though is that a user may pass multiple fragments. At that point do we also provide the user a fragmentName option, do we only take the first fragment (I could see this leading to implicit errors), or do we throw an error?

An alternative would be to introduce a special cacheNode type, which just selects that particular cache node.

@helfer What would that look like?

[stubailo]

You mean a special root field that only Apollo knows about? That seems much less clean than using a fragment (we could enforce that you call that fragment main, or add an @main directive for example).

I'm currently pretty convinced that we should never encourage people to write GraphQL query strings on the client that aren't valid with regards to their server-side schema.

[stubailo]

Note that this will break basically any tool that expects to look at the client code and find valid queries, such as the recently released persist-graphql tool, every single editor integration, etc.

[calebmer]

I think it is ideal if we can use fragments, but we’d need to add a fragmentName option. Then we also have to decide if we want to allow an operationName option as well.

[stubailo]

Personally I think a fragmentName/operationName option, along with a default of using the first fragment, would be pretty nice. Using the first fragment in the document just seems intuitively what I would expect, especially if we think some significant proportion of usage will be with exactly one fragment.

[calebmer]

I don’t like the idea of implicitly using the first fragment. It feels like a source of issues. I’m all for automatically using the fragment if there is only one, though 👍

[stubailo]

Yeah I suppose if we just print a nice error message when fragmentName is not provided, then people will quickly realize they need that!

[calebmer]

In the future, I think the ideal solution is to refactor gql-tag so that it only returns a single AST object at once instead of entire documents. This approach would avoid operationNames and fragmentNames, be more type-safe, simpler to implement/understand, and would align with our API for importing from GraphQL files using graphql-tag/loader (I assume at the moment we can import individual fragments and operations from .graphql files. If not then I want that too 😉). Such an API would look like:

const fragment1 = gql.fragment`fragment on Foo { ... }`;
const fragment2 = gql.fragment`fragment on Bar { ...${fragment1} }`;
const query = gql.operation`query { ...${fragment2} }`;
const document = gql`mutation { ... } query { ... }`;

client.readFragment(fragment1, 'Foo-4');

…or with imports:

import { fragment1 } from './MyQuery.graphql';

client.readFragment(fragment1, 'Foo-4');

For now, though, what I did instead was break client.read into two functions. client.readQuery and client.readFragment. It turns out that it’s really hard to do duck-typing and validation in a single method. Also, there are subtle differences between the two. For example, client.readFragment does not take variables whereas client.readQuery does. client.readQuery never takes an id. It will always read from ROOT_QUERY. client.readFragment, however, always takes an id.

[calebmer]

If we decide we like this pattern, then we should probably use it to split write into writeQuery and writeFragment as well. This may also mean adding writeQueryOptimistically and writeFragmentOptimisitcally.

[helfer]

@calebmer: why does readFragment never take variables? Personally I would prefer just having two functions rather than four or even six.

For writing to the store, the story is a bit more complicated, because we have to do that through Redux actions, we can't do it directly.

[calebmer]

readFragment does not take variables because currently variables are never allowed in fragments per the GraphQL spec. There is some discussion about adding variables access to fragments, however.

I’m perfectly comfortable with six different methods (or more!) than trying to fit those six methods awkwardly into two 😊. It’s good API design because it’s monomorphic (so more performant), easier to type for TypeScript (so errors get caught before runtime), more sensible to a user (they know exactly when to use a fragment and when to use a query), and simpler to implement (I tried doing it the other way, and the combinatorial explosion of invariants got pretty crazy).

We could pack it into one Redux action for writing to the store, though. The user would get a nice API and we would massage that API into one consistent write action.

[stubailo]

does not take variables because currently variables are never allowed in fragments per the GraphQL spec.

That's not the case:

fragment myFragment on MyType {
  field(arg: $var) {
    otherField
  }
}

Is a perfectly valid GraphQL fragment!

[calebmer]

@stubailo oh wow. I didn’t know variables were implicitly global (see this query). I’ll add the variables option back.

[stubailo]

Yeah there are dozens of pages of conversations somewhere about why variables are global per query, and apparently this was an intentional decision in the spec.

[calebmer]

I remember reading through the issue about adding explicit local fragment variables, and I remember seeing global fragment variables as an option. I never thought to check how variables were actually handled in fragments and assumed the specification punted that decision to a later point in time instead of doing the implicit global thing.

[calebmer]

Renaming the branch unfortunately closed this issue. I’m also going to add the write method to this branch so I will reopen when that is implemented.