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

Update mergeSchemas and delegation API, plus Schema Transfroms #527

Merged
merged 67 commits into from
Apr 23, 2018

Conversation

freiksenet
Copy link
Contributor

@freiksenet freiksenet commented Dec 8, 2017

This adds new API for delegation, that includes GraphQL Transforms.

Squashed commit of the following:

commit a108877910e0556759f92e9acab4643c63be13ff
Author: Mikhail Novikov <[email protected]>
Date:   Wed Jan 31 16:05:00 2018 +0200

    First transforms

commit 334802c
Merge: 99884b6 e9c0eb5
Author: Mikhail Novikov <[email protected]>
Date:   Fri Jan 26 12:27:57 2018 +0200

    Merge remote-tracking branch 'origin/master' into next-api

commit 99884b6
Author: Mikhail Novikov <[email protected]>
Date:   Fri Jan 26 11:57:15 2018 +0200

    Restored valid fragments

commit f1b2432
Author: Mikhail Novikov <[email protected]>
Date:   Thu Jan 25 14:55:57 2018 +0200

    Progress :P

commit db2806a
Author: Mikhail Novikov <[email protected]>
Date:   Thu Jan 25 13:35:35 2018 +0200

    Fragment replacement

commit e9c0eb5
Author: Mikhail Novikov <[email protected]>
Date:   Wed Jan 24 18:28:12 2018 +0200

    v2.19.0 (#594)

commit e33c215
Author: Mikhail Novikov <[email protected]>
Date:   Wed Jan 24 16:31:37 2018 +0200

    Progress

commit 464670f
Author: Mikhail Novikov <[email protected]>
Date:   Tue Jan 23 15:03:40 2018 +0200

    More progress

commit 22d2295
Author: Sebastian Richter <[email protected]>
Date:   Tue Jan 23 12:45:49 2018 +0100

    Also recreate astNode for fields (#580)

    * Also recreate astNode for fields

    In a [previous commit](fd9f626) we added the `astNode` property in the `reacreateCompositeType` function. That resulted in cache control working with schema stitching but only for GraphQL Types. By recreating the `astNode` prop also in `fieldToFieldConfig` cache control also works for fields. This is required for caching fields and hence queries.

    * Add ast to input field node too

commit 03ad432
Author: Renato Benkendorf <[email protected]>
Date:   Tue Jan 23 08:35:00 2018 -0200

    Fix resolvers to accept and move forward args with zero or false values (#586)

    Fixing the args with zero value or false

commit 72ac16f
Author: Mikhail Novikov <[email protected]>
Date:   Tue Jan 23 11:37:02 2018 +0200

    Upgrade remap-istanbul (#590)

commit 5420557
Merge: 24a0f3f 3ef557a
Author: Sashko Stubailo <[email protected]>
Date:   Mon Jan 22 11:55:07 2018 -0800

    Merge pull request #584 from shackpank/double_deps

    Remove graphql-subscriptions from devDependencies

commit 3ef557a
Author: Ollie Buck <[email protected]>
Date:   Fri Jan 19 14:40:03 2018 +0000

    Remove graphql-subscriptions from devDependencies

commit 7e4efe9
Author: Mikhail Novikov <[email protected]>
Date:   Thu Jan 18 16:38:13 2018 +0200

    Progress

commit 24a0f3f
Author: Mikhail Novikov <[email protected]>
Date:   Wed Jan 10 12:36:43 2018 +0200

    v2.18.0 (#574)

commit 966e102
Author: Amit-A <[email protected]>
Date:   Wed Jan 10 02:53:56 2018 -0500

    Fixing broken links (#573)

commit 94b8f68
Author: Pascal Kaufmann <[email protected]>
Date:   Tue Jan 9 12:51:53 2018 +0100

    Fix fragment filtering edge case when a type implements multiple interfaces (#546)

    * add failing test for multi interface issue

    * interface / interface always implements an abstract type

    * changelog

    * the linter always should be happy

    * Update CHANGELOG.md

commit 608414b
Author: Tim Griesser <[email protected]>
Date:   Tue Jan 9 06:15:12 2018 -0500

    Allow IEnumResolver values to be number type (#568)

    * Allow IEnumResolver values to be number type

commit 6177034
Author: Mikhail Novikov <[email protected]>
Date:   Tue Jan 9 11:26:04 2018 +0200

    v2.17.0 (#571)

commit fd9f626
Author: Sebastian Richter <[email protected]>
Date:   Tue Jan 9 09:25:16 2018 +0100

    Include astNode in schema recreation (#569)

    * Update schemaRecreation.ts

    `mergeSchemas` does not set `astNode` properties, when recreating types.
    But `CacheControlExtension.willResolveField` uses the `astNode` property in order to get the `cacheControl` directives.

commit 4f489d3
Author: Mikhail Novikov <[email protected]>
Date:   Wed Jan 3 15:40:21 2018 +0200

    v2.16.0 (#564)

commit c4c5d91
Author: VimalRaj Selvam <[email protected]>
Date:   Wed Jan 3 19:00:16 2018 +0530

    #544 - Update docs and tests to reflect new strings-as-descriptions pattern in new GraphQL SDL (#559)

    * #64 - Update docs and tests to reflect new strings-as-descriptions pattern in new GraphQL SDL

    * Make tests compatible with graphql v0.11

    * Fix test errors

    * Remove extra space

    * Fix review comments

commit 66d58bb
Author: Alvis Tang <[email protected]>
Date:   Wed Jan 3 13:25:59 2018 +0000

    fix: correct the dependency issue in typescript caused by #445 (#561)

    fix #472

commit 342bece
Author: Johannes Schickling <[email protected]>
Date:   Wed Jan 3 14:21:35 2018 +0100

    Add subscriptions support to `makeRemoteExectuableSchema` (#563)

    Added subscriptions support to makeRemoteExectuableSchema

commit 8e8e348
Author: Mikhail Novikov <[email protected]>
Date:   Tue Jan 2 14:50:06 2018 +0200

    v2.15.0 (#562)

commit f2905ac
Author: Johannes Schickling <[email protected]>
Date:   Tue Jan 2 13:40:56 2018 +0100

    Add document validation to `delegateToSchema` (#551)

    * Add document validation to `delegateToSchema`

commit 59592e1
Merge: 3e24c69 3612216
Author: Sashko Stubailo <[email protected]>
Date:   Thu Dec 28 10:02:28 2017 -0800

    Merge pull request #556 from enaqx/patch-4

    doc: fix Generating schema URL on Mocking page

commit 3612216
Author: Sashko Stubailo <[email protected]>
Date:   Thu Dec 28 10:00:16 2017 -0800

    Update mocking.md

commit 3e24c69
Merge: 7089bbf 42d180d
Author: Sashko Stubailo <[email protected]>
Date:   Thu Dec 28 09:58:31 2017 -0800

    Merge pull request #557 from tbroadley/fix-typos

    docs: fix typos

commit 42d180d
Merge: 5130cf6 7089bbf
Author: Sashko Stubailo <[email protected]>
Date:   Thu Dec 28 09:57:11 2017 -0800

    Merge branch 'master' into fix-typos

commit 7089bbf
Merge: 3a58286 5048434
Author: Sashko Stubailo <[email protected]>
Date:   Thu Dec 28 09:52:44 2017 -0800

    Merge pull request #552 from mklopets/patch-2

    Fix link to Apollo Link docs

commit 5048434
Author: Marko Klopets <[email protected]>
Date:   Thu Dec 28 14:23:07 2017 +0200

    Re-fix link

commit 5130cf6
Author: Thomas Broadley <[email protected]>
Date:   Mon Dec 25 17:45:44 2017 -0500

    docs: fix typos

commit 80f2f4c
Author: Nick Raienko <[email protected]>
Date:   Mon Dec 25 09:19:03 2017 +0200

    doc: fix Generating schema URL on Mocking page

commit e295019
Author: Marko Klopets <[email protected]>
Date:   Thu Dec 21 13:18:59 2017 +0200

    Fix link to Apollo Link docs

commit 3a58286
Merge: 259f22e 971e96e
Author: Sashko Stubailo <[email protected]>
Date:   Wed Dec 20 11:26:55 2017 -0800

    Merge pull request #548 from apollographql/create-2.14.1

    v2.14.1

commit 971e96e
Author: Mikhail Novikov <[email protected]>
Date:   Wed Dec 20 12:44:14 2017 +0200

    v2.14.1

commit 259f22e
Author: Mikhail Novikov <[email protected]>
Date:   Wed Dec 20 12:42:16 2017 +0200

    Guard against schemas without QueryType (#547)

commit 1e3bb14
Author: Mikhail Novikov <[email protected]>
Date:   Tue Dec 19 14:06:13 2017 +0200

    Check null resolvers

commit 2e2c9b1
Author: Mikhail Novikov <[email protected]>
Date:   Mon Dec 11 16:09:06 2017 +0200

    Simplify API
@stubailo
Copy link
Contributor

Todo:

  • Add test for calling delegateToSchema directly
  • Add a nice error for when the wrong number of arguments is passed, since that means people probably forgot to pass in the name of the schema
  • Documentation and blog post about transforms

import { recreateType, createResolveType } from '../stitching/schemaRecreation';

export enum VisitSchemaKind {
TYPE = 'VisitSchemaKind.TYPE',
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's call this ANY_TYPE

import { propertySchema } from './testingSchemas';
import makeSimpleTransformSchema from '../transforms/makeSimpleTransformSchema';

function RenameTypes(renameMap: { [originalName: string]: string }): Transform {
Copy link
Contributor

Choose a reason for hiding this comment

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

We could make this more general and also work for namespacing if this took a function instead of an object:

(type) => ({
  oldName: 'newName'
}[type])

(type) => ('NameSpace' + type)

@stubailo
Copy link
Contributor

Proposed list of transforms:

  1. Rename types
  2. Rename fields
  3. Select types/fields (opt-in)
  4. (maybe) remove types/fields (opt-out)

@freiksenet freiksenet changed the title Schema transforms, new mergeSchemas and everything Update mergeSchemas and delegation API, plus Schema Transfroms Apr 18, 2018
@sync
Copy link
Contributor

sync commented Apr 19, 2018

Can confirm that the problem with fragment into fragment we were experiencing is now fixed. Thank you once more.

@freiksenet
Copy link
Contributor Author

Let's merge it folks @benjamn @stubailo

freiksenet and others added 8 commits April 20, 2018 13:31
This implementation returns new objects only if modifications were made,
and avoids defining/exporting a generic abstraction for functionality that
was only used once.
Note that I have removed the sections about visitSchema and visitObject,
since those utilities are no longer exported by the graphql-tools package.
@martijnwalraven
Copy link
Contributor

This is great, happy to see we're getting close to a release! I know I haven't really been involved with this, so I'm a bit late to the party, but some questions that came up while looking at this:

  • Why do we export the built-in transforms under a Transforms object, instead of having people import them individually? Apart from the unusual capitalization, I feel it may draw too much of a distinction between built-in and custom transforms.
  • Since we're changing the API anyway here, would it make sense to try and come up with a better name than mergeInfo that we can also use to put additional utility methods on (like accessing a schema by name)? Maybe just utils or tools?

@freiksenet
Copy link
Contributor Author

@martijnwalraven I'll change the transforms export. Our API is backwards compatible, so I think we are stuck with mergeInfo, I don't think it's worth a rename, esp if we have to keep compatability layer.

@martijnwalraven
Copy link
Contributor

Oh, I thought we were changing the API from a mergeInfo closure argument to mergeInfo on info. I don't want to bikeshed this too much, but I'm afraid mergeInfo doesn't cover what we want to use this for so if we have an opportunity to fix it I think we should do that.

We'll probably want to add a resolveSchema method for instance, that resolves a schema by name. It seems weird for that to live on mergeInfo. What do you think of alternative names? I think something like tools or stitching makes the most sense to me.

@freiksenet
Copy link
Contributor Author

Moved transform export to top level. Also added two transforms that a) were reported by users b) I needed for my blog post (:D)

}

if (value && typeof value === 'object') {
const newObject = Object.create(Object.getPrototypeOf(value));
Copy link
Contributor

@benjamn benjamn Apr 20, 2018

Choose a reason for hiding this comment

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

Note: I changed this code so that it now handles arrays as well as any other kind of object, preserving the original prototype. I think it's important to transform arrays as well as objects here, since you could have objects nested inside array results that need their __typenames to be updated.

export { default as FilterRootFields } from './FilterRootFields';
export { default as ExpandAbstractTypes } from './ExpandAbstractTypes';
export { default as ExtractField } from './ExtractField';
export { default as WrapQuery } from './WrapQuery';
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this new style a lot better, thanks!

}

if (options.operation === 'subscription') {
// apply result processing ???
Copy link
Contributor

@benjamn benjamn Apr 23, 2018

Choose a reason for hiding this comment

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

I forgot to submit a comment from a couple weeks ago (sorry), and this code has changed a bit since then, but I think the question is still relevant: is // apply result processing ??? an important TODO?

Since we're planning to bump the graphql-tools major version to 3.0.0, I
think it's fine to hard-deprecate passing positional arguments to
delegateToSchema, by throwing an exception rather than warning/continuing.
There's no reason to keep using the old style, especially now that the
fragmentReplacements argument has been replaced by transforms.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants