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

feat: Add new execution handler #921

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kristoferma
Copy link

Adds feature mentioned by me in #782

I called the handler executeGraphQL, but there is probably a better name for it.

This enables me to execute GraphQL queries in Next.js GetServerSideProps without using a http server.

I only tested this with my project

TODO

  • docs
  • tests

@kristoferma kristoferma changed the title Add new execution handler feat: Add new execution handler May 26, 2020
Copy link
Member

@jasonkuhrt jasonkuhrt left a comment

Choose a reason for hiding this comment

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

Thanks @kristoferma! The main thing I think we need to discuss is how to handle the lack of request data in the overall framework API. For example schema.addToContext calls currently expect it.

Because this is a non-trivial design problem, please open a new issue to discuss it and let's keep further discussion here light for now. Thanks!

-- edit

One crazy idea I'm having right now– is something like this:

  • Statically analyze resolvers
  • generate static data about which do and do not depend on the request object
  • augment schema.exec to only see a sub-graph of the whole API, that part which does NOT depend on request object data.

I'll copy this over to the issue once you've created the issue @kristoferma

operation
) => {
const source = new Source(query)
const context = await contextCreator({})
Copy link
Member

@jasonkuhrt jasonkuhrt May 27, 2020

Choose a reason for hiding this comment

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

Hm, how is this going to work out?

AFAICT it requires that no resolvers in your app rely on data indirectly or directly from the request object.

If any do, there could be an error not caught until runtime.

This is the main design problem here to discuss I think.

@@ -81,6 +82,48 @@ export const createRequestHandlerGraphQL: CreateHandler = (schema, createContext
return sendSuccess(res, result)
}

export type ExecuteGraphQl = (
Copy link
Member

Choose a reason for hiding this comment

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

Ideally this function is used within the createRequestHandlerGraphQL

@@ -60,6 +61,13 @@ export function create(appState: AppState) {
}) ?? noop
)
},
get executeGraphQL() {
Copy link
Member

@jasonkuhrt jasonkuhrt May 27, 2020

Choose a reason for hiding this comment

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

Executing a graphql query in its pure form seems like something that should be tied to the schema component, not the server component.

For example I could imagine this:

schema.exec(...)

@jasonkuhrt jasonkuhrt added the needs/discussion Open-ended conversation about something (ideation, design, analysis, ...) label May 27, 2020
@netlify
Copy link

netlify bot commented Aug 17, 2020

Deploy request for graphql-nexus pending review.

Review with commit b51b562

https://app.netlify.com/sites/graphql-nexus/deploys

@kristoferma
Copy link
Author

apollo-server-express reduced the amount of code needed to expose an execute handler from nexus.

I am more than willing to put some effort in adding documentation to this feature if you can agree on my implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs/discussion Open-ended conversation about something (ideation, design, analysis, ...)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants