[WIP] Transfrorm schema API by IvanGoncharov · Pull Request #1199 · graphql/graphql-js

IvanGoncharov · 2018-01-10T17:14:03Z

As discussed in #1185 I rewrote sort function to sort GraphQLSchema.
In a process, I notice that sortSchema duplicates > 50% of code from extendSchema so I decided to extract common part.
After this change, extendSchema became way simpler and #1095 trivial to implement.
To make this PR easier to review I split it into two commits one implementing sortSchema and another changing extendSchema.

@leebyron What do you think?

langpavel · 2018-01-11T02:39:53Z

Hi @IvanGoncharov Can you pick up use case for this?
I see it sorts even enum values. I hope that this cannot be dangerous even in any badly taken implementation..

IvanGoncharov · 2018-01-16T14:07:37Z

Can you pick up use case for this?

@langpavel See #941

I hope that this cannot be dangerous even in any badly taken implementation..

We are living in the dangerous world 😆 Seriously, the order of things shouldn't make any difference and even if for some weird reason you depend on it just don't use lexographicSortSchema on your schema.

IvanGoncharov · 2018-01-16T15:32:06Z

I renamed this function to lexographicSortSchema based on #941 (comment)

IvanGoncharov · 2018-01-16T15:49:39Z

+/**
+ * Note: This class is not a part of public API and is a subject to changes
+ * in the future.
+ */


I think implementing #922 and #1095 will be the real test for this API and may require some changes so I don't want to freeze this API

IvanGoncharov · 2018-01-16T15:58:25Z

@leebyron Can you please give a quick look at this PR?
This PR has some non-trivial changes (especially SchemaTransformer class) so I'm not sure if I'm on a right track.

leebyron

There's a lot happening in this PR that I think the quality could be improved by breaking it into smaller separate diffs. My suggested progression:

First, if the goal is a schema sorting function, I'd suggest building that directly. Then I'd suggest building this much more complex schema transformer API. Finally, I'd suggest using that transformer API to replace implementations of each function that makes sense.

That should be at least 3-4 separate PRs, each of which can use a previous PR as a base to make them reviewable as a stack

leebyron · 2018-01-18T20:00:22Z

+ * Note: This class is not a part of public API and is a subject to changes
+ * in the future.
+ */
+export class SchemaTransformer {


I'm not sure why this is a class since the only think you can do with it is instance.transformSchema(). I'd suggest refactoring this to just export a function transformSchema(schema, transformer) since that presents the same behavior in a simpler API

leebyron · 2018-01-18T20:01:41Z

+} from '../type/definition';
+
+type ExactSchemaConfig = $Exact<GraphQLSchemaConfig> & {
+  types: Array<*>,


We should avoid * as much as possible - Flow often cannot resolve them, so these are either effectively all any or they may be derived to some type which could produce an error that is very difficult to understand. They appear a lot in this file, I think almost every case should be able to be replaced with a true type reference

leebyron · 2018-01-18T20:02:58Z

+  isInputObjectType,
+} from '../type/definition';
+
+type ExactSchemaConfig = $Exact<GraphQLSchemaConfig> & {


Why $Exact all of the config types?

leebyron · 2018-01-18T20:11:53Z

+    typeRef => {
+      invariant(schemaTransformer);
+      const typeName = typeRef.name.value;
+      const type = schemaTransformer.transformType(typeName);


This API is a bit confusing - this isn't a transform function, it's something that takes a name and return a type, so I'd expect this to be more like the getType() API

IvanGoncharov · 2018-01-29T21:33:05Z

That should be at least 3-4 separate PRs, each of which can use a previous PR as a base to make them reviewable as a stack

@leebyron After #1208 being merged what is a next PR I should extract?
Maybe it worth to add toConfig method into every GraphQL*Type class since it useful on its own and will simplify implementation of transformSchema.

What do you think about adding toConfig and convert both lexographicSortSchema' and extendSchema` to use it as a next PR?

leebyron · 2018-01-29T21:40:12Z

That could definitely be a good next step - I'll leave it to you - it's also fine to build up multiple PRs, having each cite the previous as a way to illustrate a path of changes

mapSchema enables transformation of schema objects without modification of the original schema. See graphql/graphql-js#1199

yaacovCR · 2020-05-12T21:00:40Z

Inspired by this prior art, see now fleshed out mapSchema in ardatan/graphql-tools#1463

One implementation consideration I had that might be useful, right now FieldMappers get a fieldConfig and can return a fieldConfig, but TypeMappers get a type and have to return a type, so that a given object type could actually be converted to a scalar as desired and vice a versa.

This could be changed to eliminate that unlikely use case. Alternatively separate OBJECT_TYPE and OBJECT_TYPE_CONFIG mappers could be used. At this point, I am happy with retaining the discrepancy, as it maintains flexibility with decreased API surface, but it does require some repetitive

function mapperFunction(originalConfig) {
  const originalConfig = objectType.toConfig();
  ....<create a new config based on old>...
  return new GraphqlObjectType(newConfig)
}

instead of

function mapperFunction(originalConfig) {
  ....<create a new config based on old>...
  return newConfig
}

IvanGoncharov · 2021-08-10T10:16:32Z

Extracted in #3226

facebook-github-bot added the CLA Signed label Jan 10, 2018

IvanGoncharov mentioned this pull request Jan 16, 2018

Type extension does not work ardatan/graphql-import#42

Closed

IvanGoncharov force-pushed the sortSchema branch from 8c41043 to ce927c4 Compare January 16, 2018 15:27

IvanGoncharov changed the title ~~Sort schema~~ lexographicSortSchema Jan 16, 2018

IvanGoncharov commented Jan 16, 2018

View reviewed changes

leebyron requested changes Jan 18, 2018

View reviewed changes

IvanGoncharov mentioned this pull request Jan 22, 2018

Add 'lexographicSortSchema' #1208

Merged

IvanGoncharov changed the title ~~lexographicSortSchema~~ [WIP] Transfrorm schema API Jan 22, 2018

lillyfwang mentioned this pull request Feb 2, 2018

Allow interface type extensions #1222

Merged

OlegIlyenko mentioned this pull request Feb 4, 2018

Extract generic functionality from schema materializer sangria-graphql/sangria#333

Open

IvanGoncharov mentioned this pull request Feb 8, 2018

Make 'ASTDefinitionBuilder' responsible only for build types from AST #1230

Merged

IvanGoncharov added 2 commits February 21, 2018 21:55

Add 'lexographicSortSchema' function

6b051ab

Reuse transformSchema inside extendSchema

ed02120

IvanGoncharov force-pushed the sortSchema branch from ce927c4 to ed02120 Compare February 21, 2018 20:36

IvanGoncharov mentioned this pull request Sep 7, 2018

Upgrade graphql peerDependency to 14.0.0 ardatan/graphql-tools#945

Closed

IvanGoncharov mentioned this pull request Mar 7, 2019

[Proposal] Add addField method for object type #1774

Closed

IvanGoncharov mentioned this pull request May 9, 2019

Add utility function function for generating fully-featured schema from SDL #1858

Open

IvanGoncharov force-pushed the master branch from e7509a6 to 16f5ca0 Compare June 26, 2019 17:05

yaacovCR added a commit to yaacovCR/graphql-tools-fork that referenced this pull request Mar 9, 2020

feat(mapSchema): initial version

dfe07b6

mapSchema enables transformation of schema objects without modification of the original schema. See graphql/graphql-js#1199

yaacovCR added a commit to yaacovCR/graphql-tools-fork that referenced this pull request Mar 10, 2020

feat(mapSchema): initial version

b107e91

mapSchema enables transformation of schema objects without modification of the original schema. See graphql/graphql-js#1199

yaacovCR added a commit to yaacovCR/graphql-tools-fork that referenced this pull request Mar 26, 2020

feat(mapSchema): initial version

77b4053

mapSchema enables transformation of schema objects without modification of the original schema. See graphql/graphql-js#1199

yaacovCR added a commit to yaacovCR/graphql-tools-fork that referenced this pull request Mar 26, 2020

feat(mapSchema): initial version

98964d0

mapSchema enables transformation of schema objects without modification of the original schema. See graphql/graphql-js#1199

Base automatically changed from master to main January 27, 2021 11:10

This was referenced May 28, 2021

[Snyk] Upgrade cspell from 5.2.4 to 5.4.0 turkdevops/graphql-js#187

Closed

[Snyk] Upgrade cspell from 5.2.4 to 5.4.1 turkdevops/graphql-js#196

Closed

IvanGoncharov mentioned this pull request Aug 10, 2021

Add utility function to transform types/directives inside GraphQLSchema #3226

Closed

IvanGoncharov closed this Aug 10, 2021

falkenhawk mentioned this pull request Mar 17, 2023

Allow sorting in enums to be disabled dotansimha/graphql-code-generator#6935

Closed

Conversation

IvanGoncharov commented Jan 10, 2018

Uh oh!

langpavel commented Jan 11, 2018

Uh oh!

IvanGoncharov commented Jan 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IvanGoncharov commented Jan 16, 2018

Uh oh!

IvanGoncharov Jan 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IvanGoncharov commented Jan 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leebyron left a comment

Choose a reason for hiding this comment

Uh oh!

leebyron Jan 18, 2018

Choose a reason for hiding this comment

Uh oh!

leebyron Jan 18, 2018

Choose a reason for hiding this comment

Uh oh!

leebyron Jan 18, 2018

Choose a reason for hiding this comment

Uh oh!

leebyron Jan 18, 2018

Choose a reason for hiding this comment

Uh oh!

IvanGoncharov commented Jan 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leebyron commented Jan 29, 2018

Uh oh!

yaacovCR commented May 12, 2020

Uh oh!

IvanGoncharov commented Aug 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

IvanGoncharov commented Jan 16, 2018 •

edited

Loading

IvanGoncharov Jan 16, 2018 •

edited

Loading

IvanGoncharov commented Jan 16, 2018 •

edited

Loading

IvanGoncharov commented Jan 29, 2018 •

edited

Loading