Skip to content

[RFC] Add explicit context arg to graphql execution#326

Merged
leebyron merged 1 commit into
masterfrom
context
Mar 25, 2016
Merged

[RFC] Add explicit context arg to graphql execution#326
leebyron merged 1 commit into
masterfrom
context

Conversation

@leebyron
Copy link
Copy Markdown
Contributor

This BREAKING change introduces a new argument to the GraphQL execution API which is presented to resolution functions: context.

This solves a long-standing point of confusion about the correct way to represent authentication or a "viewer context" in which a query is executed.

Previously, we suggested that the rootValue contain any authentication tokens, however this led to awkward code:

resolve: (val, args, { rootValue: { authToken } }) { ... }

Which can now be written as:

resolve: (val, args, authToken) { ... }

The info object is still created and provided to resolution functions, and the rootValue is still provided within it, however it is now the fourth argument rather than the third.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.0004%) to 99.385% when pulling 188f8be on context into 43992e3 on master.

@freiksenet
Copy link
Copy Markdown
Contributor

You probably meant

resolve: (val, args, { authToken }) { ... }

Right?

@freiksenet
Copy link
Copy Markdown
Contributor

I like the change otherwise, it clears the slightly confusing "root value as context" pattern. Also destructuring in resolve became much easier.

@JeffRMoore
Copy link
Copy Markdown
Contributor

I think its a good idea, not for the de-structuring sugar, but to give each value a single responsiblity.

This *BREAKING* change introduces a new argument to the GraphQL execution API which is presented to resolution functions: `context`.

This solves a long-standing point of confusion about the correct way to represent authentication or a "viewer context" in which a query is executed.

Previously, we suggested that the `rootValue` contain any authentication tokens, however this led to awkward code:

```
resolve: (val, args, { rootValue: { authToken } }) { ... }
```

Which can now be written as:

```
resolve: (val, args, authToken) { ... }
```

The `info` object is still created and provided to resolution functions, and the `rootValue` is still provided within it, however it is now the *fourth* argument rather than the *third*.
@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.0003%) to 99.421% when pulling d7cc6f9 on context into 576b6a1 on master.

@leebyron leebyron merged commit 942f84e into master Mar 25, 2016
@leebyron leebyron deleted the context branch March 25, 2016 00:40
@joewood
Copy link
Copy Markdown

joewood commented Apr 13, 2016

Stupid question: - why wasn't the argument added after the AST info rather than before to avoid the breaking change?

@leebyron
Copy link
Copy Markdown
Contributor Author

The breaking change is undesirable, but the final state of the API is much better. The now-fourth info argument will only very rarely be accessed, and that also allows us some opportunities for future performance tuning.

@leggsimon
Copy link
Copy Markdown

Apologies if this a stupid question, pretty new to graphQL and am updating our versions. Just to clarify,
This

...
    technology: {
      type: Page,
      resolve: (root, _, { rootValue: { flags, backend = backendReal }}) =>
        backend(flags).capi.page(sources.technology.uuid)
    },
...

Now becomes

...
    technology: {
      type: Page,
      resolve: (root, _, { flags, backend = backendReal }) =>
        backend(flags).capi.page(sources.technology.uuid)
    },
...

Is that right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants