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

[3.0.0] Regression with fragments that have fields with variables #753

Closed
xzyfer opened this issue Apr 26, 2018 · 3 comments · Fixed by #806
Closed

[3.0.0] Regression with fragments that have fields with variables #753

xzyfer opened this issue Apr 26, 2018 · 3 comments · Fixed by #806
Labels
blocking Prevents production or dev due to perf, bug, build error, etc.. bug has-reproduction ❤ Has a reproduction in a codesandbox or single minimal repository

Comments

@xzyfer
Copy link
Contributor

xzyfer commented Apr 26, 2018

We've run into an issue with fragments that have fields with variables.
This can come about when bubbling up fragments that are co-located with their component code.

Note: this is not to be mistaken with the experiment fragment variables feature.

I've done some debugging and narrowed the issue down tomergeSchemas.
Without it everything works as expected. I suspect something isn't get merged correctly.

Setup

Minimal reproduction setup
const gql = require('graphql-tag');
const makeExecutableSchema = require('graphql-tools').makeExecutableSchema;
const mergeSchemas = require('graphql-tools').mergeSchemas;
const SchemaLink = require('apollo-link-schema').SchemaLink;
const ApolloClient = require('apollo-client').ApolloClient;
const InMemoryCache = require('apollo-cache-inmemory').InMemoryCache;

const schema = makeExecutableSchema({
  typeDefs: `
    type Query {
      foo: Foo!
    }

    type Foo {
      name: String!
      bar(id: String!): Bar!
    }

    type Bar {
      name: String!
    }

    schema {
      query: Query
    }
  `,
  resolvers: {
    Query: {
      foo: () => ({ name: 'foo' }),
    },
    Foo: {
      bar: () => ({ name: 'bar' }),
    },
  },
});

const client = new ApolloClient({
  cache: new InMemoryCache(),
  link: new SchemaLink({
    schema: mergeSchemas({ schemas: [schema] }),
  }),
});

Execution

client.query({
  query: gql`
    query foo($id: String!) {
      foo(id: $id) {
        name
        ...BarFragment
      }
    }
    fragment BarFragment on Foo {
      bar(id: $id) {
        name
      }
    }
  `,
  variables: { id: 10 },
})
  .then(console.log)
  .catch(console.error);

Output in 2.x

> { data: { foo: { name: 'foo', bar: [Object], __typename: 'Foo' } },
  loading: false,
  networkStatus: 7,
  stale: false }

Output in 3.0.0-beta.0

> { Error: GraphQL error: Variable "$id" is not defined.
    at new ApolloError (/app/node_modules/apollo-client/bundle.umd.js:121:28)
    at /app/node_modules/apollo-client/bundle.umd.js:1187:41
    at /app/node_modules/apollo-client/bundle.umd.js:1620:17
    at Array.forEach (<anonymous>)
    at /app/node_modules/apollo-client/bundle.umd.js:1619:18
    at Map.forEach (<anonymous>)
    at QueryManager.broadcastQueries (/app/node_modules/apollo-client/bundle.umd.js:1614:22)
    at Object.next (/app/node_modules/apollo-client/bundle.umd.js:1660:31)
    at notifySubscription (/app/node_modules/zen-observable-ts/node_modules/zen-observable/lib/Observable.js:126:18)
    at onNotify (/app/node_modules/zen-observable-ts/node_modules/zen-observable/lib/Observable.js:161:3)
  graphQLErrors:
   [ { Error: Variable "$id" is not defined.
    at asErrorInstance (/app/node_modules/graphql/execution/execute.js:541:43)
    at <anonymous>
    at runMicrotasksCallback (internal/process/next_tick.js:121:5)
    at _combinedTickCallback (internal/process/next_tick.js:131:7)
    at process._tickDomainCallback (internal/process/next_tick.js:218:9)
       message: 'Variable "$id" is not defined.',
       locations: [],
       path: [Array] } ],
  networkError: null,
  message: 'GraphQL error: Variable "$id" is not defined.',
  extraInfo: undefined }
@ghost ghost added blocking Prevents production or dev due to perf, bug, build error, etc.. bug has-reproduction ❤ Has a reproduction in a codesandbox or single minimal repository labels Apr 26, 2018
@xzyfer xzyfer changed the title [3.0.0.beta.0] Regression with fragments that have fields with variables [3.0.0-beta.0] Regression with fragments that have fields with variables Apr 26, 2018
@ghost ghost added blocking Prevents production or dev due to perf, bug, build error, etc.. bug has-reproduction ❤ Has a reproduction in a codesandbox or single minimal repository labels Apr 26, 2018
@freiksenet
Copy link
Contributor

Sigh I've just moved those around to fix another bug.

Here is the problem: https://github.com/apollographql/graphql-tools/blob/master/src/transforms/FilterToSchema.ts#L100-L105

Fragments do usedVariables after operations.I guess a second pass on operations is needed to fix that.

@xzyfer xzyfer changed the title [3.0.0-beta.0] Regression with fragments that have fields with variables [3.0.0] Regression with fragments that have fields with variables May 3, 2018
@ghost ghost added blocking Prevents production or dev due to perf, bug, build error, etc.. bug has-reproduction ❤ Has a reproduction in a codesandbox or single minimal repository labels May 3, 2018
@xzyfer
Copy link
Contributor Author

xzyfer commented May 3, 2018

I've bisected the regression to 663c9af

xzyfer pushed a commit to xzyfer/graphql-tools that referenced this issue May 25, 2018
A regression was introduced in 3.0.0-beta.0 (663c9af) and shipped
in 3.0.0 which caused an error if a fragment field argument used
a variable defined in the parent query.

Fixes ardatan#753
xzyfer pushed a commit to xzyfer/graphql-tools that referenced this issue May 25, 2018
A regression was introduced in 3.0.0-beta.0 (663c9af) and shipped
in 3.0.0 which caused an error if a fragment field argument used
a variable defined in the parent query.

Fixes ardatan#753
xzyfer pushed a commit to xzyfer/graphql-tools that referenced this issue May 25, 2018
A regression was introduced in 3.0.0-beta.0 (663c9af) and shipped
in 3.0.0 which caused an error if a fragment field argument used
a variable defined in the parent query.

Fixes ardatan#753
xzyfer pushed a commit to xzyfer/graphql-tools that referenced this issue Jun 5, 2018
A regression was introduced in 3.0.0-beta.0 (663c9af) and shipped
in 3.0.0 which caused an error if a fragment field argument used
a variable defined in the parent query.

Fixes ardatan#753
@hwillson
Copy link
Contributor

#806 was merged (thanks @xzyfer!), and released in graphql-tools 3.0.3. Thanks all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocking Prevents production or dev due to perf, bug, build error, etc.. bug has-reproduction ❤ Has a reproduction in a codesandbox or single minimal repository
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants