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

Do not add __typename to @client selection sets @export-ed as input variables. #4784

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
76 changes: 75 additions & 1 deletion packages/apollo-client/src/__tests__/local-state/export.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import gql from 'graphql-tag';
import ApolloClient from '../..';
import { InMemoryCache } from 'apollo-cache-inmemory';
import { ApolloLink, Observable } from 'apollo-link';
import { print } from 'graphql/language/printer';

describe('@client @export tests', () => {
it(
Expand Down Expand Up @@ -156,7 +157,7 @@ describe('@client @export tests', () => {
cache.writeData({
data: {
currentAuthor: testAuthor,
}
},
});

return client.query({ query }).then(({ data }: any) => {
Expand Down Expand Up @@ -572,6 +573,79 @@ describe('@client @export tests', () => {
},
);

it('should not add __typename to @export-ed objects (#4691)', () => {
const query = gql`
query GetListItems($where: LessonFilter) {
currentFilter @client @export(as: "where") {
title_contains
enabled
}
lessonCollection(where: $where) {
items {
title
slug
}
}
}
`;

const expectedServerQuery = gql`
query GetListItems($where: LessonFilter) {
lessonCollection(where: $where) {
items {
title
slug
__typename
}
__typename
}
}
`;

const currentFilter = {
title_contains: 'full',
enabled: true,
};

const data = {
lessonCollection: {
__typename: 'LessonCollection',
items: [
{
__typename: 'ListItem',
title: 'full title',
slug: 'slug-title',
},
],
},
};

const client = new ApolloClient({
link: new ApolloLink(request => {
expect(request.variables.where).toEqual(currentFilter);
expect(print(request.query)).toBe(print(expectedServerQuery));
return Observable.of({ data });
}),
cache: new InMemoryCache({
addTypename: true,
}),
resolvers: {
Query: {
currentFilter() {
return currentFilter;
},
},
},
});

return client.query({ query }).then(result => {
expect(result.data).toEqual({
currentFilter,
...data,
});
});
});

it(
'should use the value of the last @export variable defined, if multiple ' +
'variables are defined with the same name',
Expand Down
30 changes: 19 additions & 11 deletions packages/apollo-utilities/src/transform.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {
} from './getFromAST';
import { filterInPlace } from './util/filterInPlace';
import { invariant } from 'ts-invariant';
import { isField, isInlineFragment } from './storeUtils';

export type RemoveNodeConfig<N> = {
name?: string;
Expand Down Expand Up @@ -228,15 +229,26 @@ export function addTypenameToDocument(doc: DocumentNode): DocumentNode {
// introspection query, do nothing.
const skip = selections.some(selection => {
return (
selection.kind === 'Field' &&
((selection as FieldNode).name.value === '__typename' ||
(selection as FieldNode).name.value.lastIndexOf('__', 0) === 0)
isField(selection) &&
(selection.name.value === '__typename' ||
selection.name.value.lastIndexOf('__', 0) === 0)
);
});
if (skip) {
return;
}

// If this SelectionSet is @export-ed as an input variable, it should
// not have a __typename field (see issue #4691).
const field = parent as FieldNode;
if (
isField(field) &&
field.directives &&
field.directives.some(d => d.name.value === 'export')
Copy link
Member Author

Choose a reason for hiding this comment

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

@hwillson Should this test be stricter? For example, we could require that the directive has an as argument that refers to a declared variable.

Copy link
Member

Choose a reason for hiding this comment

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

@benjamn I think it's okay like this. If we need to change this in the future, we should be able to pretty easily.

Copy link

Choose a reason for hiding this comment

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

This does not work in the case where the field contains nested fields:

filters @client @export(as: "filters") {
  priceFilters {
    value
  }

The typename will be removed from filters, but not from priceFilters.

I have been using this workaround, but it seems undesirable:

filters @client @export(as: "filters") {
  priceFilters @export {
    value
  }

) {
return;
}

// Create and return a new SelectionSet with a __typename Field.
return {
...node,
Expand Down Expand Up @@ -292,7 +304,7 @@ function hasDirectivesInSelection(
selection: SelectionNode,
nestedCheck = true,
): boolean {
if (selection.kind !== 'Field' || !(selection as FieldNode)) {
if (!isField(selection)) {
return true;
}

Expand Down Expand Up @@ -446,7 +458,7 @@ function getAllFragmentSpreadsFromSelectionSet(

selectionSet.selections.forEach(selection => {
if (
(selection.kind === 'Field' || selection.kind === 'InlineFragment') &&
(isField(selection) || isInlineFragment(selection)) &&
selection.selectionSet
) {
getAllFragmentSpreadsFromSelectionSet(selection.selectionSet).forEach(
Expand Down Expand Up @@ -514,12 +526,8 @@ export function removeClientSetsFromDocument(
enter(node) {
if (node.selectionSet) {
const isTypenameOnly = node.selectionSet.selections.every(
selection => {
return (
selection.kind === 'Field' &&
(selection as FieldNode).name.value === '__typename'
);
},
selection =>
isField(selection) && selection.name.value === '__typename',
);
if (isTypenameOnly) {
return null;
Expand Down