Skip to content
This repository has been archived by the owner on Mar 20, 2023. It is now read-only.

Add support for customParseFn option #484

Merged
merged 4 commits into from
May 11, 2019

Conversation

williambailey
Copy link
Contributor

Attempt to address #483.

The current implementation in this PR provides support for synchronous customParseFn functions.

Eventually I think that it would be worthwhile also supporting asynchronous functions. i.e. have the customParseFn return type be a (let's see if i get the Flow type correct) ?Promise<DocumentNode>. Unfortunitely I couldn't quite get the promise handling .then().catch() logic right at this point in time... even when requiring the customParseFn be async (vs. maybe async).

@IvanGoncharov
Copy link
Member

@williambailey Thanks for PR 👍

Eventually I think that it would be worthwhile also supporting asynchronous functions. i.e. have the customParseFn return type be a (let's see if i get the Flow type correct) ?Promise. Unfortunitely I couldn't quite get the promise handling .then().catch() logic right at this point in time... even when requiring the customParseFn be async (vs. maybe async).

You don't need async since options callback can return promise of options:

graphqlHTTP(graphqlHTTPOptions);

function graphqlHTTPOptions(request, response, params) {
  return getASTFromCache(params.query).then(ast => {
    schema,
    customParseFn() {
      return ast;
    }
  })
}

Every time you add .then it forces the node to switch to the next task in even loop and this adds performance penalty so it makes sense to batch all async jobs in options callback.
Also, it makes source code easier to read and reason about.

Note: you also get params inside options callback so it doesn't make sense to change the signature of parse from graphql-js. So can you please change customParseFn to:

customParseFn?: ?(source: Source) => DocumentNode,

@williambailey
Copy link
Contributor Author

Completely forgot that you could return a promise of options. PR updated.

src/index.js Outdated
@@ -265,11 +272,11 @@ function graphqlHTTP(options: Options): Middleware {
}

// GraphQL source.
const source = new Source(query, 'GraphQL request');
const source = new Source(query);
Copy link
Member

Choose a reason for hiding this comment

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

@williambailey Can you please revert this line?

@IvanGoncharov IvanGoncharov merged commit 604f8e6 into graphql:master May 11, 2019
@IvanGoncharov
Copy link
Member

@williambailey Thanks 👍
Merged 🎉
I will try to release NPM 📦 next week.

@IvanGoncharov IvanGoncharov added the PR: feature 🚀 requires increase of "minor" version number label May 15, 2019
@IvanGoncharov
Copy link
Member

@williambailey Sorry for delay. I just published 0.9.0 📦

junminstorage pushed a commit to junminstorage/express-graphql that referenced this pull request Aug 14, 2020
* Add support for customParseFn option
* Update README.md
* Make customParseFn have the same signature as parse()
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
PR: feature 🚀 requires increase of "minor" version number
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants