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

Add Fragment Spread Arguments #2871

Closed

Conversation

daniel-nagy
Copy link

@daniel-nagy daniel-nagy commented Dec 27, 2020

This is a continuation of the work done by @sam-swarr in #1141 to support parameterized fragments. This PR adds the ability to pass arguments to fragment spreads. For example,

query GetEmployee($id: ID!, $employeeScheduleFilters: ScheduleFilters) {
  employee(id: $id) {
    ...EmployeeFragment(scheduleFilters: $employeeScheduleFilters)
  }
}

fragment EmployeeFragment($scheduleFilters: ScheduleFilters) on Employee {
  id
  name
  schedule(filters: $scheduleFilters) {
    # selection set
  }
}

Parameterized fragments are a prerequisite for encapsulation. Without encapsulation fragments are context dependent. This makes them difficult to compose into larger selection sets or to reuse in more than one operation.

In my opinion each fragment spread (not inline fragment) should create a new isolated scope. However, this would be a breaking change. I think a good compromise is to allow opting-in to isolated scopes by adding variables to a fragment. Each parameterized fragment would create a new isolated scope and fields within that fragment cannot use variables outside that fragment's scope.

The following is an example of transforming a document that contains parameterized fragments into a spec compliant document that can be parsed by GraphQL servers.

import {
  ArgumentNode,
  ASTNode,
  FragmentDefinitionNode,
  Kind,
  VariableDefinitionNode,
  ValueNode,
  VariableNode,
  visit,
} from './npmDist/language';

type NonEmptyArray<T> = [T, ...T[]];

interface VariableArgumentNode extends ArgumentNode {
  value: VariableNode;
}

interface VariableFragmentDefinitionNode extends FragmentDefinitionNode {
  variableDefinitions: Readonly<NonEmptyArray<VariableDefinitionNode>>;
}

type VariableScope = {
  [key: string]: ValueNode;
};

type Context = {
  parameterizedFragments?: VariableFragmentDefinitionNode[];
  variables?: VariableScope;
};

type Predicate<T, U extends T> = (item: T) => item is U;

const isVariableArgument = (arg: ArgumentNode): arg is VariableArgumentNode =>
  arg.value.kind === Kind.VARIABLE;

const isVariableFragmentDefinition = (
  node: ASTNode,
): node is VariableFragmentDefinitionNode =>
  node.kind === Kind.FRAGMENT_DEFINITION &&
  Boolean(node.variableDefinitions?.length);

const splitBy = <T, U extends T>(
  items: readonly T[],
  predicate: Predicate<T, U> | ((item: T) => boolean),
): typeof predicate extends Predicate<T, U>
  ? [Exclude<T, U>[], U[]]
  : [T[], U[]] =>
  items.reduce<[T[], U[]]>(
    ([left, right], item) =>
      predicate(item) ? [left, [...right, item]] : [[...left, item], right],
    [[], []],
  );

/**
 * Transform a graphql document that contains parameterized fragments into
 * a spec compliant document that graphql servers can parse.
 *
 * Directives on fragment variables are not supported. Hoisting variable
 * directives would allow one fragment to produce side effects outside its scope.
 *
 * Each parameterized fragment will create a new isolated scope. Field arguments
 * within a parameterized fragment cannot use variables outside of the fragment's
 * scope.
 *
 * Fragment spreads will be replaced with inline fragments to allow using the
 * same fragment more than once, in a single operation, with different arguments.
 *
 * Parameterized fragments may contain more parameterized fragments and each
 * nested parameterized fragment will create a new isolated scope.
 */
const visitWithContext = (
  root: ASTNode,
  visited: WeakSet<ASTNode> = new WeakSet(),
  context?: Context,
) => {
  const newRoot = visit(root, {
    enter(node) {
      if (visited.has(node)) {
        return false;
      }
    },
    Document(node) {
      const [definitions, parameterizedFragments] = splitBy(
        node.definitions,
        isVariableFragmentDefinition,
      );

      const warnAboutDirectives = parameterizedFragments.some(
        ({ variableDefinitions }) =>
          variableDefinitions.some(({ directives }) => directives?.length),
      );

      if (warnAboutDirectives) {
        console.warn('Directives on fragment variables are not supported.');
      }

      return {
        ...node,
        definitions: [
          ...definitions.map((definition) =>
            visitWithContext(definition, visited, { parameterizedFragments }),
          ),
        ],
      };
    },
    Field(node) {
      if (!node.arguments?.length || !context?.variables) {
        return node;
      }

      const [args, variableArgs] = splitBy(node.arguments, isVariableArgument);

      return {
        ...node,
        arguments: [
          ...args,
          ...variableArgs.map((arg) => ({
            ...arg,
            value: context.variables?.[arg.value.name.value],
          })),
        ],
      };
    },
    FragmentSpread({ arguments: args, ...node }) {
      const fragment = context?.parameterizedFragments?.find(
        (fragment) => fragment.name.value === node.name.value,
      );

      if (!fragment) {
        return node;
      }

      // Replace the fragment with an inline fragment. This is necessary to
      // allow the same fragment to be used more than once with different
      // variables in the same operation.
      return {
        directives: [
          ...(node.directives ?? []),
          ...(fragment.directives ?? []),
        ],
        kind: Kind.INLINE_FRAGMENT,
        loc: undefined,
        selectionSet: visitWithContext(fragment.selectionSet, visited, {
          ...context,
          variables: fragment.variableDefinitions.reduce(
            (variables, node) => ({
              ...variables,
              [node.variable.name.value]:
                args?.find((arg) => arg.name.value === node.variable.name.value)
                  ?.value ?? node.defaultValue,
            }),
            {},
          ),
        }),
        typeCondition: fragment.typeCondition,
      };
    },
  });

  visited.add(newRoot);
  return newRoot;
};

If the queries are statically analyzable then this transformation can be applied at compile time. If this transformation was applied to the above example the result would be,

query GetEmployee($id: ID!, $employeeScheduleFilters: ScheduleFilters) {
  employee(id: $id) {
    ... on Employee {
      id
      name
      schedule(filters: $employeeScheduleFilters) {
        # selection set
      }
    }
  }
}

Base automatically changed from master to main January 27, 2021 11:10
@daniel-nagy daniel-nagy force-pushed the experimental-fragment-arguments branch from 9fa2fb2 to 4590519 Compare March 20, 2021 18:13
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Mar 20, 2021

CLA Signed

The committers are authorized under a signed CLA.

@daniel-nagy
Copy link
Author

I rebased this on main. It looks like in the time between when I first opened this PR and now fragment variables have been deprecated. Has this proposal been completely abandoned? Is there more information on the reason for deprecating?

@mjmahone
Copy link
Contributor

mjmahone commented Jun 1, 2021

@daniel-nagy: I am not terribly good at git, so couldn't easily figure out how to take your commits, rebase them on master, and stack my own local commits on top of them. I'm sure there's a way, I'm just not used to git and need to learn how.

But in the interest of moving the conversation along, I pulled in your changes and then added the executor changes needed: #3152

I think this is a good starting point. I'd be happy to work with you on the next steps here, but I think we need to add validation rules next.

Without encapsulation fragments are context dependent. This makes them difficult to compose into larger selection sets or to reuse in more than one operation.

This is a much bigger problem than merely fragment parameterization. To solve this, we'd probably need to solve to allow the same field to be resolved with different arguments at the same level. That will be challenging, especially to support in a way that's backwards compatible.

@IvanGoncharov
Copy link
Member

Closing in favor of #3152

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

Successfully merging this pull request may close these issues.

4 participants