Graphql-tools: Why should i merge resolvers manually when typedefs are automatically merge ?

[julestruong]

I can pass my schemas in an array in typeDefs, but resolvers should be merged before ?

const resolvers = merge({}, RootQueryResolver, CharacterResolver, DateTimeResolver, GenderResolver);

const schema = makeExecutableSchema({ 
    typeDefs: [SchemaDefinition, RootQuery, Character, DateTime, Gender],
    resolvers,
});

Why am i not being able to do so ?

 const schema = makeExecutableSchema({ 
    typeDefs: [SchemaDefinition, RootQuery, Character, DateTime, Gender],
    resolvers: [RootQueryResolver, CharacterResolver, DateTimeResolver, GenderResolver],
});

Just by curiosity , why is that ?

@neoziro

[stubailo]

You know, that's a good question, and there's not really a good reason! I'd definitely accept a PR for this :]

We're trying to avoid a dependency on lodash, though, so maybe just copy the implementation of merge, or write a simple one that just knows how to merge resolvers!

[gregberge]

@julestruong do you plan to make a PR? Else I am OK to do it.

[julestruong]

@neoziro be my guest :)

[kbrandwijk]

I would also love this on mergeSchemas for consistency.

[stubailo]

Leeeeeeeet's do it! @neoziro excited about that PR :]

[o-alexandrov]

Why not to simply use a spread operator for the resolvers Object?

I have not confirmed whether minificators with addition of proper scope hoisting compile it into a static object containing all resolvers. However, if it is still unsupported, it will be soon and running a lodash function when there is ES6 syntax available, IMHO, should be avoided.

// ./dynamodb/resolvers.js
export default {
  Query: {
    dynamoGetUser: (root, payload) => dynamoTweets(payload)
  },
  Mutation: {
    addNewEmail: (root, payload) => addNewEmail(payload)
  }
}

// schema.js

import rDynamo from './dynamodb/resolvers'

... // typeDefs

export default makeExecutableSchema({
  typeDefs,
  resolvers: {
    ...rDynamo
  },
})

P.S. modularization of typeDefs should also be improved in the docs #520

[stubailo]

@OlzhasAlexandrov: The problem is that doesn't merge the whole tree:

r1 = {
 Query: {
    field1: () => {}.
  }
}

r2 = {
 Query: {
    field2: () => {}.
  }
}

resolvers = {
  ...r1,
  ...r2
}

// Ends up with only field2

If you use merge though, you'll get all the resolvers.

[o-alexandrov]

@stubailo Yes that's true. My previous example contains only one object that I pass.
Obviously one would need to pass it a bit differently:

r1 = {
 Queries: {
    field1: () => {}.
    field2: () => {}.
    field3: () => {}.
  }
 Mutations: {
    field1: () => {}.
  }
}

r2 = {
 Queries: {
    field4: () => {}.
  }
 Mutations: {
    field2: () => {}.
  }
}

resolvers = {
  Query: {
    ...r1.Queries,
    ...r2.Queries,    
  }
  Mutation: {
    ...r1.Mutations,
    ...r2.Mutations,  
  }
}

In the end, I again emphasize:

I have not confirmed whether minificators with addition of proper scope hoisting compile it into a static object containing all resolvers. However, if it is still unsupported, it will be soon and running a lodash function when there is ES6 syntax available, IMHO, should be avoided.

Running functions to combine typeDefs and resolvers when it could be statically compiled would have a noticeable expense when there are hundreds of thousands users.

[stubailo]

Do you know of any tools that convert the above into a statically compiled result? I don't think using ... is any faster than merge. I think when talking about performance it's critical to refer to real data, could we get some?

[o-alexandrov]

@stubailo

Overall, IMHO, even if there is no tool to prepack the objects before minification occurs at the moment, it is architecture-wise not the best solution to leave it to a function to merge objects.

At the moment I checked only babel-minify and it doesn't convert it to a static object
However, babel-minify is still in beta and it is a collection of minification plugins. Some time soon the spread operator will be better handled by minificators.

The dead code elimination should/will take out unnecessary intermediate variables. Using functions as we see in this project couldn't possibly achieve that.

I will check later today the google closure compiler on objects, but it does work on string concatenation with advanced optimization.

EDIT: the reason why I am bringing the string concatenation back is #520.

[stubailo]

Personally I'm not willing to modify the API without concrete data about performance improvements, since performance speculation can lead to a dangerous place.

[o-alexandrov]

@stubailo
The source code is already in ES6+. How would using object spreads cause any havoc besides the removal of unneeded (if using ES6 for everything) functions?

I apologize if I am not familiar with the rules of the repository on Issues and turning it into a discussion thread. Also, I am extremely grateful for the package overall.

[stubailo]

I mean, this is more a suggestion to use something else in documentation, right? And I think in the above example it's clear that merge does something that the spread doesn't, which is exactly why we chose it.

[gregberge]

Right now I don't have time to do it. So I will not submit a PR. Sorry!

[mfix22]

@stubailo I can submit this PR, and I can probably rewrite merge in graphql-tools, but just to confirm you guys don't want to depend on lodash.merge either, right?

[frederikhors]

@mfix22 is still lodash.merge today right? I can read it here: https://www.apollographql.com/docs/link/links/state.html#organize or it is a mistake?

[mfix22]

lodash.merge is still valid, although graphql-tools will now merge your resolvers automatically, so you don't need lodash.merge if you are just trying to merge resolvers.

[frederikhors]

Can you write an example, I don't understand.

I'm reading this:

import merge from 'lodash.merge';
import { withClientState } from 'apollo-link-state';

import currentUser from './resolvers/user';
import cameraRoll from './resolvers/camera';
import networkStatus from './resolvers/network';

const stateLink = withClientState({
  cache,
  resolvers: merge(currentUser, cameraRoll, networkStatus),
});

Can I avoid lodash here?

[mfix22]

@frederikhors

import { withClientState } from 'apollo-link-state';

import currentUser from './resolvers/user';
import cameraRoll from './resolvers/camera';
import networkStatus from './resolvers/network';

const stateLink = withClientState({
  cache,
  resolvers: [currentUser, cameraRoll, networkStatus]
});

This works for makeExecutableSchema, not sure if it works for withClientState. @hwillson want me to implement that in apollo-link-state like I did for graphql-tools?

[frederikhors]

@mfix22 thanks a lot. But what I think is it doesn't work with array. On my project it doesn't work, maybe this doesn't work for apollo-link-state's withClientState:

See here: https://github.com/apollographql/apollo-link-state/blob/master/packages/apollo-link-state/src/index.ts

export type ClientStateConfig = {
  cache?: ApolloCache<any>;
  resolvers: any | (() => any);
  defaults?: any;
  typeDefs?: string | string[] | DocumentNode | DocumentNode[];
  fragmentMatcher?: FragmentMatcher;
};

[mfix22]

@frederikhors sorry yeah that makes sense. In that case merge should work for you. If the Apollo team wants, I can implement it for withClientState

[frederikhors]

I think it's not easy.

But where would you do it, in the project repo: https://github.com/apollographql/apollo-link-state?

I opened an issue there: https://github.com/apollographql/apollo-link-state/issues/300