Add support for customValidateFn and rename formatError to customFormatErrorFn

[jamesmoriarty]

Regarding: #474

[coveralls]

Coverage increased (+0.02%) to 99.396% when pulling 4522dd9 on newscorpaus:feature/validation-cache into 7ab2bc9 on graphql:master.

[jamesmoriarty]

@IvanGoncharov aka graphql overlord can you please review?

[jamesmoriarty]

Hey @IvanGoncharov, the set cache property signature was a mistake. I've changed it to set(key, value) and get(key). I've changed the test to use LruMap.

[ivome]

I also think wrapping / replacing the validate function would be better. I wrote a validation rule to analyze the query complexity and reject complex queries that depends on the input variables: graphql-query-complexity
To use something like this with validation caching we would have to implement some custom logic.

Maybe we could even add a plugin system to express-graphql to have a well defined interface for things like this and even more complex extensions? This could also be used for #391.

Initialization could look something like this:

app.get('/graphql', graphqlHTTP({
  schema: MyGraphQLSchema,
  graphiql: true,
  plugins: [
    new ValidationCachePlugin(options),
    // Other plugins
    new FakeResultPlugin(),

    // We could also add hooks for things like...
    new ProfilingPlugin(),
    // etc.
  ]
}));

Might be overkill for just this one feature and this would definitely need some more detailed conception but I just wanted to throw this out here to see what you think...

[jamesmoriarty]

Thanks for the feedback @IvanGoncharov. Using the same pattern as the formatErrorworked great.

[IvanGoncharov]

@jamesmoriarty Thanks 👍
Can you also cleanup test cases (it doesn't make sense to introduce new dependencies just for tests) and make it more like formatError/#391?

[jamesmoriarty]

Removed unnecessary dependencies and cleaned up the tests. Looks consistent with formatError and your execute PR.

[IvanGoncharov]

@jamesmoriarty Thanks a lot 👍 I will try to merge it in the next few days.

One issue I still have is with both validate and execute is that they are not very intuitive names for the functionality they enable.
Maybe replaceValidate/replaceExecute?
Would be great to hear your thoughts?

[jamesmoriarty]

I agree. The existing implementation made it possible. I've had a look at the graphql-js source to get ideas on consistency. I'm thinking customValidateFn customExecuteFn and possible deprecate formatError and change to customFormatErrorFn. What do you think @IvanGoncharov?

Based on:

[IvanGoncharov]

@jamesmoriarty Great suggestion 👍 I like it.
I will update #391 to use customExecuteFn.

I'm thinking customValidateFn customExecuteFn and possible deprecate formatError and change to customFormatErrorFn

Make sense 👍
I think we should support deprecated formatError until 1.0.0 release.
Do you want to work on PR for this?

[jamesmoriarty]

Sure thing.

[jamesmoriarty]

Made the change to the naming, deprecating formatError and updating the README. I imagine there is a better way to notify the user with the deprecation warning than using console.error. Any feedback appreciated.

[jamesmoriarty]

@IvanGoncharov interestingly I couldn't change this to:

optionsData.customFormatErrorFn || optionsData.formatError || formatError
// ...
.map(errorFormatFn) 

Without breaking the tests. It appears the

.map(errorFormatFn || formatError) 

may be called before the options resolve. maybe around schema validation.

Might be worth resolving outside this change.

[jamesmoriarty]

Added to README. Thanks for spending time on this @IvanGoncharov. Much appreciated.

[IvanGoncharov]

@jamesmoriarty I made a few small fixes like replacing console.error with console.warn and fixing ESLint warnings.