Graphql-js: Accept an object as the argument to graphql()

[taion]

This came up in the context of https://github.com/graphql/graphql-relay-js/pull/74#issuecomment-209108125, but I think that given the number of arguments to graphql(): https://github.com/graphql/graphql-js/blob/v0.5.0/src/graphql.js#L43-L50, it would make more sense for it to take an object rather than a list of arguments.

Among other things, this would make it easier to provide backward compatibility with the v0.4.x API, and also just make it less error-prone for those of us not using e.g. Flow to get all the arguments right.

[helfer]

Couldn't agree more. Should be relatively easy work for a PR

[taion]

Yup! Question is – would it be acceptable to do this with a backward compat hook such that the old 0.4.x API (without context) still works? I feel like that would make it significantly easier (along with other backward compat affordances) for users to move forward. I haven't upgraded yet, given that graphql-relay hasn't released yet, but I'm not looking forward to the upgrade.

[helfer]

I personally don't think it's worth complicating the code going forward to keep back-compatibility with 0.4.0. I'd prefer having a clean solution for future upgrades, but maybe that's just because I don't yet have anything in production using graphql.

[helfer]

Another thing this proposal would let us do is pass an option to graphql which tells it to format errors with the default function. I find it quite inconvenient to have to format them myself now.

[helfer]

To add to what was said above: at this point making the interface backwards-compatible would require making it backwards-compatible with both v0.5.* and v0.4.*, which frankly I don't think is even possible because the arguments are optional. If someone passes three arguments, there's no way of knowing whether that third argument should be interpreted as the root value or as the context.

[sheerun]

I'm all for changing api to options object

[leebyron]

I think this is a good idea, but am curious how it would work. Right now the first argument to graphql is schema which itself is an object. We would just have to be quite careful to ensure such a change was non breaking.

[taion]

I think the idea would be that this change would in fact be breaking (or maybe we can check for various properties on the object), but it would allow future changes like adding context as in 0.5.0 to be non-breaking.

[SunnyGurnani]

This should really be done otherwise we will keep breaking the API's

[b-barry]

After going through this issue, I would like to propose the solution below.
First, I create a new Object type for Object as the argument to graphql()

  type ObjectArgumentGraphql{
    schema: GraphQLSchema,
    requestString: string,
    rootValue?: mixed,
    contextValue?: mixed,
    variableValues?: ?{[key: string]: mixed},
    operationName?: ?string
  }

Then rename the first argument schema to options ( at the moment I don't find a real good name so if you have some idea, it is will be great if you can share it :) ). This argument will use the Union type system. They will have type GraphQLSchema or ObjectArgumentGraphql.
And based on the type of the first argument, I can determine if you have to destructure or not the argument to provide to the real "graph function" .

export function graphql(
  options: GraphQLSchema | ObjectArgumentGraphql ,
  requestString: ? string,
  rootValue?: mixed,
  contextValue?: mixed,
  variableValues?: ?{[key: string]: mixed},
  operationName?: ?string
): Promise<GraphQLResult> {

  if(typeof options === 'ObjectArgumentGraphql'){
    let { schema, requestString, rootValue , contextValue , variableValues, operationName } = options;
    return graph( schema, requestString, rootValue , contextValue , variableValues, operationName);
  }

  return graph( schema, requestString, rootValue , contextValue , variableValues, operationName);
}

function graph(
  schema: GraphQLSchema ,
  requestString:  string,
  rootValue?: mixed,
  contextValue?: mixed,
  variableValues?: ?{[key: string]: mixed},
  operationName?: ?string
): Promise<GraphQLResult> {

  return new Promise(resolve => {
    const source = new Source(requestString || '', 'GraphQL request');
    const documentAST = parse(source);
    const validationErrors = validate(schema, documentAST);
    if (validationErrors.length > 0) {
      resolve({ errors: validationErrors });
    } else {
      resolve(
        execute(
          schema,
          documentAST,
          rootValue,
          contextValue,
          variableValues,
          operationName
        )
      );
    }
  }).catch(error => {
    return { errors: [ error ] };
  });
}

What do you think about it ?

[migueloller]

A sensible approach could be to make the change and when graphql is called take a look at the arguments. If the caller is using the old (multiple arguments) signature, simply log a deprecation warning.

Implementing this change would be quite simple since the first argument is easily identifiable (i.e., arg isntanceof GraphQLSchema).

graphql(
  schema,
  requestString,
  rootValue,
  contextValue,
  variableValues,
  operationName
); // console.warn(deprecationReason);

graphql({
  schema,
  requestString,
  rootValue,
  contextValue,
  variableValues,
  operationName
}); // 👍

It would be a very simple change and I'd be willing to make a PR if it's something @leebyron would consider merging.

EDIT: As everybody else has been commenting, this change would be very good because of two main reasons: (1) There is no specific argument order meaning that the developer could skip optional arguments (or the implementation could be simplified if it allows for optional arguments) and (2) as new arguments are added to the graphql function, changes are less likely to be breaking given that it will still take an object and the order doesn't matter.

[taion]

IMO it'd make the most sense to just go ahead and break the API. It's what was done for context.