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

Execute: use consistent name for execution context #1318

Closed
wants to merge 3 commits into from

Conversation

IvanGoncharov
Copy link
Member

It helps to easily distinguish between user-provided context and execution context.

@@ -378,9 +378,9 @@ export function buildExecutionContext(
*/
function executeOperation(
exeContext: ExecutionContext,
operation: OperationDefinitionNode,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing function signature was easier to understand - executeOperation() should accept an operation.

@leebyron
Copy link
Contributor

Similarly, user-provided context should always appear as contextValue - I don't think that consistency is there

leebyron added a commit that referenced this pull request Apr 23, 2018
commit b58697d8e91de91265cbebe48906e0c29b48b28b
Author: Lee Byron <lee@leebyron.com>
Date:   Mon Apr 23 16:50:02 2018 -0400

    Also unify contextValue

commit 30d062cedd687c1f662d9e87e08d35d61c2a50ce
Merge: 1d07ebf 4db01f3
Author: Lee Byron <lee@leebyron.com>
Date:   Mon Apr 23 16:47:31 2018 -0400

    Merge branch 'exeContextName' of https://github.com/APIs-guru/graphql-js into APIs-guru-exeContextName

commit 4db01f3
Author: Lee Byron <lee@leebyron.com>
Date:   Mon Apr 23 13:43:20 2018 -0700

    Addendum

commit 571f2bc
Author: Lee Byron <lee@leebyron.com>
Date:   Mon Apr 23 13:42:41 2018 -0700

    Address review

commit 0195b30
Author: Ivan Goncharov <ivan.goncharov.ua@gmail.com>
Date:   Sat Apr 21 17:30:35 2018 +0300

    Execute: use consistent name for execution context
@leebyron
Copy link
Contributor

I merged in the contextValue change as well :)

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.

3 participants