Fragment variables fix

[GreenGremlin]

Currently, the graphql-anywhere graphql function always tries to rectify directives, even when it's being used to walk a fragment to generate propTypes or filter result data. This change updates filter and check (used by propType) to pass a null value for the variable mapping. The change also updates the graphql function's variableValues argument to default to an empty map, which is how it currently functions, when no value is passed. This allows the graphql function to function exactly as it currently does, for pretty much every scenario. Except, if null variableValue is explicitly passed it will not process @include and @Skip directives.

I think this is the best possible fix for handling fragments with variable directives.

Checklist:

• If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
• Make sure all of the significant new logic is covered by tests

Fixes #4372

[apollo-cla]

@GreenGremlin: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

[GreenGremlin]

I've signed the contributor agreement. Is there anything else I can do to help get this merged?

[GreenGremlin]

@benjamn how do you feel about catching the "Invalid variable referenced" error and ignoring it, when no variables object has been passed in?

[GreenGremlin]

Is there anything I can do to help get this merged?

[GreenGremlin]

It looks like invariant error messages are now stripped in production builds, so this fix will no longer work.

[GreenGremlin]

I updated my change so that it will work without the invariant message.

[GreenGremlin]

I refactored my fix so that the functionality of graphql-anywhere's graphql is unchanged, when either no variables are passed or an empty variable mapping is passed. Instead, it now skips processing include/skip directives when passed a null variable mapping argument. I've also updated filter and propType/check to pass a null variable mapping argument, which now allows them to be run on fragments that have @import or @Skip directives with variable arguments.

[GreenGremlin]

It would be great to hear from the Apollo team on this PR. I've invested quite a bit of time to come up with a very backwards compatible fix.

[hwillson]

Thanks for working on this @GreenGremlin.

The only test in your PR that fails against master without any of your changes is the can filter data for fragments with variables test, so let's focus on that one. What you're proposing is that when this fragment:

fragment foo on Foo {
  alias: name
  height(unit: METERS)
  avatar @include(if: $foo) {
    square
  }
}

is filtered against this data:

const data = {
  alias: 'Bob',
  name: 'Wrong',
  height: 1.89,
  avatar: {
    square: 'abc',
    circle: 'def',
    triangle: 'qwe',
  },
};

it should ignore the @include(if: $foo) directive completely by default, and just return:

{
  alias: 'Bob',
  height: 1.89,
  avatar: {
    square: 'abc',
  },
}

I'm not sure I agree with this. If you try this on master right now, you get a Invalid variable referenced in @include directive error, which in my mind is correct. If I'm attempting to filter a data set using a fragment that has variables, I want those variables to be considered. Otherwise the filtered result I get back could break my app's business requirements.

Instead of adjusting the code to essentially nullify defined directives, I'm wondering if a better option here would be to adjust filter to accept a variable map. It could then pass that variable map to its graphql call, the directive would use the defined variables properly, and you'd get back a filtered data set that follows the defined fragment business rules. So using the fragment and data above, running:

filter(fragmentWithAVariable, data, { foo: true });

would bring back the expected filtered data, without an error.

Let me know your thoughts. Again I'm just a bit concerned that with your changes we'd be essentially providing a way for filter to circumvent business rules, which could lead to unexpected application behaviour.

[GreenGremlin]

@hwillson, good call! I've updated filter to take a variable mapping, which it passes to graphql.

[GreenGremlin]

@hwillson how would you feel about also updating propTypes to take a second argument, mapPropsToVariables? Then it could pass the result of mapPropsToVariables(props) to check as the variables map.

[GreenGremlin]

I went ahead and made the update to add a mapPropsToVariables argument to propType. My only concern is that variables are often based on more than just props.

Here are some use cases that I'm not quite sure how to handle:

1. Variables based on state. I don't think state is available to custom propType checkers, otherwise we could just pass state to mapPropsToVariables.
2. For fragments, variables may be defined in a a parent component and not passed as props.

The only option I can think of, for these scenarios, is to pass () => null for mapPropsToVariables and then have the propType validation consider fields with @include/skip directives to not be required fields. Not a great solution, but it may be the best option available.

Also, I'd like to document the mapPropsToVariables argument, but the only doc where I see propType mentioned is in the recompose pattern doc. Should we add a doc for propType, or is there a better place to add it?

[GreenGremlin]

Sorry to be a pest, but I'd love to see this merged, if possible. Let me know if there's anything else I can do to help move it along.

[GreenGremlin]

@hwillson could I please get an update on where this PR stands? It's pretty disheartening to put the work in and not hear back.

[hwillson]

Sorry for the delay here @GreenGremlin, and thanks very much for working on this! We'll have this merged shortly.

[hwillson]

Thanks very much for working on this @GreenGremlin!