GraphQL v15 config via extensions

[airhorns]

This implements the new way that GraphQL asks user land code to decorate the schema: the extensions property of types and field configs. graphql-js as of v14 no longer supports being able to decorate the actual GraphQLObjectType or field objects with arbitrary properties because extra properties from the config objects passed to the constructors aren't assigned to the resulting instance. Instead, they accept an extensions option that does make it through and can contain whatever we like.

So, join-monster's non-standard properties should go in extensions I think! That way, we can rely on the graphql-js contract of them coming along instead of join-monster breaking as graphql-js changes it's private implementation. Config would come in like extensions: { sqlTable: ... }} instead of { sqlTable: ... }, and then be accessed via field.extensions.sqlTable instead of field.sqlTable.

This change would fix things like #352, without needing workarounds.

The big issue with this is that it's a big breaking change for users of join-monster: they need to update their schemas to use extensions: { sqlTable: ... }} instead of { sqlTable: ... }. This PR implements the code change but not the docs change because I wanted to get some feedback before fully committing to that. Do y'all think that a breaking change and a major version bump is alright? If you want to use graphql > 14, you should bump to join-monster 3.0 , or do we need to support backwards compatibility for all graphql versions? I think the extensions API is a lot cleaner and should be recommended for sure and I think lowering the maintenance burden for this open source project by just saying "if you want to use new graphqls you need to use new join-monsters" and not having to support all of them simultaneously, but that's not really my call to make.

[airhorns]

Just a note too, the TypeScript types for the extensions array are blocked on this from GraphQL: graphql/graphql-js#2465 , we need them to change the type to an interface in order to extend it.

[lorensr]

Thanks for this PR! Breaking is fine. I’m thinking we should namespace extensions as Ivan suggests in the types PR? So extensions: { joinMonster: { sqlTable: ... } }. Also can remove jm from:

jmIgnoreAll?: boolean 
jmIgnoreTable?: boolean

[airhorns]

Ok, this is updated to nest under a joinMonster: extensions config, and I updated all the docs as well!

[delaneyj]

I been actively trying to use this PR's repo/branch but the /dist folder isn't part of the artifacts so not an actual modules yet. Going in a npm i && npm build in node_modules/join-monster seems to work but get errors like

: "Cannot use GraphQLScalarType \"Int\" from another module or realm.\n\nEnsure that there is only one instance of \"graphql\" in the node_modules\ndirectory. If different versions of \"graphql\" are the dependencies of other\nrelied on modules, use \"resolutions\" to ensure only one version is installed.

adding

"resolutions": {
    "join-monster/graphql":"15.2.0"
  },

doesn't seem to help and its odd because i downgraded from 15.3.0 to 15.2.0 with no luck in the my package.json.

[airhorns]

@delaneyj are you using npm or yarn? Only yarn supports resolutions AFAIK.

[delaneyj]

@delaneyj are you using npm or yarn? Only yarn supports resolutions AFAIK.

Yes I am using yarn, though your project uses npm so a did pushd node_modules/join-monster && npm i && npm run build && popd. Should i try with npm instead for my project? I don't want to overtake the PR but your branch didn't have an issues and wanted to try it out as its the exact issue I'm hitting trying to use join-monster. Revert back to v13.x of graphql-js also means you lose all type information.

[airhorns]

The way I've been testing is by using yalc which is better about running the prepublishOnly and prepare scripts for linking, and the way I've been deploying is by building, adding all the files to a git branch, committing and pushing, and then installing the package from that branch. Annoying! I think there's some yarn bug where it doesn't run prepublishOnly and build the package when using resolutions, but I'm not really sure.

[delaneyj]

With @airhorns help been able to start testing this, been going through the docs and everything has been great with GraphQL v15 support! Only issue I've seen so far is at https://join-monster.readthedocs.io/en/latest/many-to-many/#applying-where-conditions where the first usage of include is used inside junction tag.

...
// add the closeness to the User instead
    closeness: {
      type: GraphQLString,
    },
 following: {
      description: 'Users that this user is following',
      type: new GraphQLList(User),
      extensions: {
        joinMonster: {
          junction: {
            // name the table that holds the two foreign keys
            sqlTable: 'relationships',
            include: {
              closeness: {
                sqlColumn: 'closeness',
              },
            },
            // filter out where they are following themselves
            where: junctionTable =>
              `${junctionTable}.follower_id <> ${junctionTable}.followee_id`,
            sqlJoins: [
              // first the parent table to the junction
              (followerTable, junctionTable, args) =>
                `${followerTable}.id = ${junctionTable}.follower_id`,
              // then the junction to the child
              (junctionTable, followeeTable, args) =>
                `${junctionTable}.followee_id = ${followeeTable}.id`,
            ],
          },
        },
      },
    },...

which gives an error of

[ERROR] 17:15:17 ⨯ Unable to compile TypeScript:
src/models/users.ts:18:3 - error TS2322: Type '() => { id: { type: GraphQLScalarType; }; email: { type: GraphQLScalarType; extensions: { joinMonster: { sqlColumn: string; }; }; }; idEncoded: { description: string; type: GraphQLScalarType; extensions: { ...; }; resolve: (user: any) => any; }; ... 6 more ...; following: { ...; }; }' is not assignable to type 'Thunk<GraphQLFieldConfigMap<any, any>>'.
  Type '() => { id: { type: GraphQLScalarType; }; email: { type: GraphQLScalarType; extensions: { joinMonster: { sqlColumn: string; }; }; }; idEncoded: { description: string; type: GraphQLScalarType; extensions: { ...; }; resolve: (user: any) => any; }; ... 6 more ...; following: { ...; }; }' is not assignable to type '() => GraphQLFieldConfigMap<any, any>'.
    Type '{ id: { type: GraphQLScalarType; }; email: { type: GraphQLScalarType; extensions: { joinMonster: { sqlColumn: string; }; }; }; idEncoded: { description: string; type: GraphQLScalarType; extensions: { ...; }; resolve: (user: any) => any; }; ... 6 more ...; following: { ...; }; }' is not assignable to type 'GraphQLFieldConfigMap<any, any>'.
      Property 'following' is incompatible with index signature.
        Type '{ description: string; type: GraphQLList<GraphQLType>; extensions: { joinMonster: { junction: { sqlTable: string; include: { closeness: { sqlColumn: string; }; }; where: (junctionTable: string) => string; sqlJoins: [...]; }; }; }; }' is not assignable to type 'GraphQLFieldConfig<any, any, { [argName: string]: any; }>'.
          The types of 'extensions.joinMonster.junction.include' are incompatible between these types.
            Type '{ closeness: { sqlColumn: string; }; }' is not assignable to type 'ThunkWithArgsCtx<{ sqlColumn?: string; sqlExpr?: string; sqlDeps?: string | string[]; }, any, { [argName: string]: any; }>'.
              Type '{ closeness: { sqlColumn: string; }; }' is not assignable to type '(args: { [argName: string]: any; }, context: any) => { sqlColumn?: string; sqlExpr?: string; sqlDeps?: string | string[]; }'.
                Type '{ closeness: { sqlColumn: string; }; }' provides no match for the signature '(args: { [argName: string]: any; }, context: any): { sqlColumn?: string; sqlExpr?: string; sqlDeps?: string | string[]; }'.

18   fields: () => ({
     ~~~~~~

  node_modules/graphql/type/definition.d.ts:445:3
    445   fields: Thunk<GraphQLFieldConfigMap<TSource, TContext>>;
          ~~~~~~
    The expected type comes from property 'fields' which is declared here on type 'Readonly<GraphQLObjectTypeConfig<any, any>>'

Wanted to know if this is not supported or is there a syntax change?

[delaneyj]

Same sort of situation happens with orderBy when used as a function instead of static object @airhorns in this section.

[airhorns]

@delaneyj Added a fix for the first issue you mentioned, but I couldn't replicate the second. I didn't actually change the TypeScript types much and the issue you're running into was there before this PR. I'd like to land this PR and then if there are still problems feel free to open a new issue.

[airhorns]

@lorensr I think this is ready to go if you folks are comfortable with the approach!

[lorensr]

Looks great! Getting an error here:

 testing typescript definitions

> join-monster@3.0.0 testtsd /Users/me/gh/join-monster
> npm run build && tsd


> join-monster@3.0.0 build /Users/me/gh/join-monster
> rm -rf dist && babel src --no-comments --out-dir dist --copy-files --ignore README.md

Successfully compiled 17 files with Babel (1910ms).

  ../../node_modules/@types/reach__router/index.d.ts:9:29
  ✖  9:29  Cannot find name Window.  

  1 error

npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! join-monster@3.0.0 testtsd: `npm run build && tsd`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the join-monster@3.0.0 testtsd script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

And can some of these be upgraded to match graphql 15?

$ npm i
npm WARN eslint-config-airbnb-base@14.1.0 requires a peer of eslint@^5.16.0 || ^6.8.0 but none is installed. You must install peer dependencies yourself.
npm WARN eslint-config-airbnb-base@14.1.0 requires a peer of eslint-plugin-import@^2.20.1 but none is installed. You must install peer dependencies yourself.
npm WARN express-graphql@0.7.1 requires a peer of graphql@^0.12.0 || ^0.13.0 || ^14.0.0 but none is installed. You must install peer dependencies yourself.
npm WARN graphql-relay@0.6.0 requires a peer of graphql@^0.5.0 || ^0.6.0 || ^0.7.0 || ^0.8.0-b || ^0.9.0 || ^0.10.0 || ^0.11.0 || ^0.12.0 || ^0.13.0 || ^14.0.0 but none is installed. You must install peer dependencies yourself.
npm WARN koa-graphql@0.8.0 requires a peer of graphql@^0.12.0 || ^0.13.0 || ^14.0.0 but none is installed. You must install peer dependencies yourself.

added 31 packages from 18 contributors, removed 1 package, updated 1 package and audited 1424 packages in 5.116s

55 packages are looking for funding
  run `npm fund` for details

found 397 vulnerabilities (395 low, 2 high)
  run `npm audit fix` to fix them, or `npm audit` for details

[lorensr]

Does 15.0.0 work? If yes, would be nicer to devs if we did ^15.0.0

[airhorns]

It would work for JS users but not for TypeScript users sadly -- the right base types for the extensions property didn't land until 15.2.0. I have a slight preference to things just working out of the box, and I don't think graphql 15->15.2 has any breaking changes.

[airhorns]

For the TypeScript issue you're getting locally, is that @types/reach_router file coming from outside the join-monster repository? That seems weird ... do you maybe have a tsconfig.json somewhere higher in your local folder tree? The tsd command passes on my machine and on travis but this stuff is so finicky I am not surprised.

For the npm warnings, sadly no, there hasn't been a graphql-relay release in two years. Once graphql/graphql-relay-js#242 is merged and released we can update though.

[delaneyj]

@airhorns sorry, I've been using your branch extensively and so far its been wonderful. I tracking down some errors but its all been me or using via graphql-compose causing some issues.

[lorensr]

Thanks a lot for this PR! Included in 3.0.0-alpha.1

For the TypeScript issue you're getting locally, is that @types/reach_router file coming from outside the join-monster repository? That seems weird ... do you maybe have a tsconfig.json somewhere higher in your local folder tree?

No, but I had a ~/node_modules, and deleting that fixed it, thanks 😄

[airhorns]

Thanks for all your feedback @lorensr !