[WIP] Extend the extend functionality by grenierdev · Pull Request #1322 · graphql/graphql-js

grenierdev · 2018-04-22T21:07:07Z

This is a WIP implementation for extending enum, input and union. See discussion in issue #1095

facebook-github-bot · 2018-04-22T21:08:40Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

IvanGoncharov · 2018-04-22T21:46:09Z

src/utilities/extendSchema.js

+ isEnumType,
+ GraphQLEnumType,
+ isInputObjectType,
+ GraphQLInputObjectType,


Please put function imports together with other function imports.

IvanGoncharov · 2018-04-22T21:50:50Z

src/utilities/extendSchema.js

+ name: type.name,
+ description: type.description,
+ fields: () => extendInputFieldMap(type),
+ astNode: type.astNode,


You need to add extensionASTNodes here. But first, you need to add it to GraphQLInputObjectType class.

IvanGoncharov · 2018-04-22T21:51:37Z

src/utilities/extendSchema.js

+ return new GraphQLEnumType({
+ name: type.name,
+ values: extendValueMap(type),
+ astNode: type.astNode,


Also missing extensionASTNodes.

IvanGoncharov · 2018-04-22T21:52:04Z

src/utilities/extendSchema.js

- types: type.getTypes().map(getExtendedType),
+ types: types,
 astNode: type.astNode,
 resolveType: type.resolveType,


Also missing extensionASTNodes.

IvanGoncharov · 2018-04-22T21:55:26Z

src/utilities/extendSchema.js

 }

+ function extendArgsMap(
+ args: GraphQLArgument[],


It accepts and returns an array so it probably shouldn't have Map in its name.

IvanGoncharov · 2018-04-22T21:57:44Z

src/utilities/extendSchema.js

+ function extendArgsMap(
+ args: GraphQLArgument[],
+ ): GraphQLArgument[] {
+ return args.map(arg => ({


I think it's better to use keyValMap here so the result of this function can be assigned to GraphQLFieldConfig::args without additional manipulations.

IvanGoncharov · 2018-04-22T22:05:45Z

src/utilities/extendSchema.js

+ const oldValueMap = Object.create(null);
+ const newValueMap = Object.create(null);
+ type.getValues().forEach(value => {
+ newValueMap[value.name] = oldValueMap[value.name] = {


I think code here should match extendFieldMap:

graphql-js/src/utilities/extendSchema.js

Lines 335 to 336 in 3cb4500

const oldFieldMap = type.getFields();

Object.keys(oldFieldMap).forEach(fieldName => {

So you either change extendFieldMap to do the same code or remove assigment chain here.

IvanGoncharov · 2018-04-22T22:10:58Z

src/utilities/extendSchema.js

+ [field],
+ );
+ }
+ newFieldMap[fieldName] = astBuilder.buildField(field);


You can't use buildField here since it return type/interface field. You should extract new function from _makeInputValues.

IvanGoncharov · 2018-04-22T22:14:44Z

src/utilities/extendSchema.js

+ newValueMap[value.name] = oldValueMap[value.name] = {
+ name: value.name,
+ description: value.description,
+ value: value.value,


You shouldn't do that, because of this check:

graphql-js/src/type/definition.js

Line 1164 in 3cb4500

value: value.hasOwnProperty('value') ? value.value : valueName,

In the current version, it will convert all enum values without explicit value to undefined.
This is the very important case so please add a separate test for it.

IvanGoncharov · 2018-04-22T22:20:55Z

@mgrenier Look nice. I left some review comments also, please fix Travis errors.
You can reproduce them by running npm t.

One important feature thing missing in this PR: Since you extending types that can be used in directive arguments that mean you need to recreate directives with updated arguments.

facebook-github-bot · 2018-04-23T15:08:59Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

grenierdev · 2018-04-26T00:32:28Z

@IvanGoncharov I still have to wrap my head around the directive argument case. Most tests cases I tried gave me errors.

extendSchema.js

function getMergedDirectives(): Array<GraphQLDirective> { const existingDirectives = schema.getDirectives(); invariant(existingDirectives, 'schema must have default directives'); return existingDirectives.concat( directiveDefinitions.map(node => astBuilder.buildDirective(node)), ).map(directive => new GraphQLDirective({ name: directive.name, description: directive.description, locations: directive.locations, args: extendArgs(directive.args), astNode: directive.astNode, })); }

extendSchema-test.js

it('extend input', () => { const extendedSchema = extendTestSchema(`  extend type Query {  newField(testArg: TestInput): String  }   input TestInput {  testInputField: String  }   directive @test(arg: TestInput) on FIELD  `);

Seems like just passing the directives args trough the newly extendArgs, function to extend args type, result in a mixup reference : Error: Schema must contain unique named types but contains multiple types named "TestInput".

I need some help on this one.

IvanGoncharov · 2018-04-26T09:51:45Z

src/utilities/extendSchema.js

 Object.keys(oldFieldMap).forEach(fieldName => {
 const field = oldFieldMap[fieldName];
 newFieldMap[fieldName] = {
+ // name: fieldName,


You don't need it ❌

IvanGoncharov · 2018-04-26T09:59:39Z

src/utilities/extendSchema.js

- description: arg.description,
- astNode: arg.astNode,
- }));
+ function extendArgs(args: GraphQLArgument[]): GraphQLFieldConfigArgumentMap {


Please use Array similar to other places in this file

IvanGoncharov · 2018-04-26T10:05:25Z

src/utilities/extendSchema.js

+ arg => arg.name,
+ arg => ({
+ name: arg.name,
+ type: extendFieldType(arg.type),


Not a critical issue but you could think of a better name for this function since now it also used for arguments. How about extendType?

extendType is already used in this scope. Should I move the logic of extendFieldType into this function or used something like extendValueType ?

@mgrenier I think it should be extendType and extendNamedType but it will make this review hard to read so I created #1327.

IvanGoncharov · 2018-04-26T10:07:56Z

src/utilities/extendSchema.js

+ args,
+ arg => arg.name,
+ arg => ({
+ name: arg.name,


You don't need name, see:

graphql-js/src/type/definition.js

Lines 842 to 847 in ae5b163

export type GraphQLArgumentConfig = {

type: GraphQLInputType,

defaultValue?: mixed,

description?: ?string,

astNode?: ?InputValueDefinitionNode,

};

IvanGoncharov · 2018-04-26T10:13:43Z

src/utilities/__tests__/extendSchema-test.js

+ const newEnum = extendedSchema.getType('SomeEnum');
+ const testDirective = extendedSchema.getDirective('test');
+
+ expect(oldEnum._values.length).to.equal(2);


nit: expect(oldEnum._values).to.have.lengthOf(2)

IvanGoncharov · 2018-04-26T10:17:49Z

src/utilities/__tests__/extendSchema-test.js

+ expect(oldEnum._values.length).to.equal(2);
+ expect(newEnum._values.length).to.equal(3);
+ expect(newEnum._values[2].name).to.equal('NEW_ENUM');
+ expect(newEnum).to.equal(testDirective.args[0].type);


nit: expect(testDirective).to.have.nested.property('args[0].type', newEnum)

IvanGoncharov · 2018-04-26T11:35:59Z

I still have to wrap my head around the directive argument case. Most tests cases I tried gave me errors.

@mgrenier You code is correct, except you shouldn't recreate new directives, only old ones:

 function getMergedDirectives(): Array<GraphQLDirective> { const existingDirectives = schema.getDirectives() ); invariant(existingDirectives, 'schema must have default directives'); return existingDirectives.concat( existingDirectives.map( directive => new GraphQLDirective({ name: directive.name, description: directive.description, locations: directive.locations, args: extendArgs(directive.args), astNode: directive.astNode, }), ), directiveDefinitions.map(node => astBuilder.buildDirective(node)), ); }

If you move out directive recreation into separate function it will make this code more readable.

Also don't forget that all code should be tested e.g.: values of extensionASTNodes, extending schema multiple times, all errors (e.g. adding duplicate enum value or input field), values of enum values, etc.
When possible it's better to extend existing tests.

IvanGoncharov · 2018-04-26T11:37:46Z

@mgrenier Also please rebase your PR on top of master, since we updated Flow & Prettier.

IvanGoncharov · 2018-04-26T11:56:18Z

src/utilities/extendSchema.js

+ newValueMap[valueName] = {
+ name: value.name,
+ description: value.description,
+ value: value.hasOwnProperty('value') ? value.value : valueName,


My bad, I got confused with something else 😞
You don't need this code, it should be value: value.value.

IvanGoncharov · 2018-04-27T11:51:05Z

@mgrenier I review you PR one more time and found that you also need to extend this function:

graphql-js/src/utilities/extendSchema.js

Line 409 in ae5b163

function checkExtensionNode(type, node) {

grenierdev · 2018-05-10T01:07:11Z

All review and rebase have been done

jgcmarins · 2018-05-10T01:55:38Z

@mgrenier it still has conflicts with src/utilities/extendSchema.js

IvanGoncharov · 2018-05-10T02:21:45Z

it still has conflicts with src/utilities/extendSchema.js

@jgcmarins @mgrenier It's minor conflict because of #1323

IvanGoncharov · 2018-05-10T02:26:54Z

src/utilities/__tests__/extendSchema-test.js

+
+ extend enum SomeEnum {
+ DEPRECATED @deprecated(reason: "do not use")
+ }


Test name is "extends objects with deprecated fields"
So I think it's better to create a separate one for enum values.

IvanGoncharov · 2018-05-10T02:33:13Z

src/utilities/__tests__/extendSchema-test.js

+ someInput: String
+ }
+
+ union UnusedUnion = Unused


I think extends schema by adding new unused types would be a better name for this test.

union UnusedUnion = Unused

Unused became used 😄 It's better to create separate DummyUnionMember type.

IvanGoncharov · 2018-05-10T02:36:46Z

src/utilities/__tests__/extendSchema-test.js

 interface NewInterface {
 buzz: String
 }
+


test description: extends objects multiple times => extends types multiple times

IvanGoncharov · 2018-05-10T02:51:45Z

@mgrenier I notice decreased coverage:
https://coveralls.io/builds/16751750/source?filename=src%2Futilities%2FextendSchema.js#L157
https://coveralls.io/builds/16751750/source?filename=src%2Futilities%2FextendSchema.js#L289
Can you please investigate and add/extend tests.

IvanGoncharov · 2018-05-10T03:06:58Z

src/utilities/extendSchema.js

 );
 }

+ function extendDirectiveType(type: GraphQLDirective): GraphQLDirective {


function name is confusing it should be either extendDirective or extendDirectiveTypes (I like it more).

I'm going with extendDirective

IvanGoncharov · 2018-05-10T03:11:01Z

src/utilities/extendSchema.js


 function getMergedDirectives(): Array<GraphQLDirective> {
- const existingDirectives = schema.getDirectives();
+ const existingDirectives = schema.getDirectives().map(extendDirectiveType);


Maybe we shouldn't recreate standard directives like @skip, @include and @deprecate.
@leebyron What do you think?

This is really elegant, so we can start with this. It would be a breaking change to disallow extending @skip/etc. but I like being able to extend them just like any other directive.

IvanGoncharov · 2018-05-10T03:16:12Z

src/utilities/extendSchema.js

+ const valueName = value.name.value;
+ if (oldValueMap[valueName]) {
+ throw new GraphQLError(
+ `Enum "${type.name}.${valueName}" already exists in the ` +


Enum => Enum value or something similar

IvanGoncharov · 2018-05-24T12:32:41Z

@mgrenier Do you need any help with this PR?
We are planning 14.0.0 and it would be great to include this PR.
I think this PR is ready it just need to be rebased and address review comments in tests.

grenierdev · 2018-06-06T01:25:35Z

@IvanGoncharov sure do. I dont have much time these days. Sorry for the delays.

If someone can dig in, feel free.

* Extend the extend functionality * Fix flowtype * Extract logic from _makeInputValues to its own function * Fix lint and minor refactors * Fix type error due to copy & paste * Fix array syntax * Fix test more verbose * Removed unnecessary check * Extend directive * Moved new tests into existing tests * Fix lint errors * Added missing description * fix enum value error test

mjmahone · 2018-06-07T21:22:03Z

This was merged in #1373

IvanGoncharov reviewed Apr 22, 2018

View reviewed changes

facebook-github-bot added the CLA Signed label Apr 23, 2018

grenierdev mentioned this pull request Apr 26, 2018

Extending input types ardatan/graphql-tools#641

Closed

IvanGoncharov reviewed Apr 26, 2018

View reviewed changes

IvanGoncharov mentioned this pull request Apr 27, 2018

extendSchema: Use consistent naming + inline 'getExtendedType' func #1327

Merged

grenierdev added 4 commits April 29, 2018 12:47

Extend the extend functionality

4b29c35

Fix flowtype

38cfdba

Extract logic from _makeInputValues to its own function

5e171de

Fix lint and minor refactors

5061360

grenierdev added 8 commits April 29, 2018 12:47

Fix type error due to copy & paste

db7d4c0

Fix array syntax

50ab294

Fix test more verbose

fed46bf

Removed unnecessary check

112e366

Extend directive

b122ea0

Moved new tests into existing tests

371b09f

Fix lint errors

5b896c6

Added missing description

b9bbe83

IvanGoncharov reviewed May 10, 2018

View reviewed changes

IvanGoncharov mentioned this pull request May 24, 2018

extendSchema incorrectly throws "multiple types" error when input type used as a query argument. #1355

Closed

IvanGoncharov mentioned this pull request Jun 4, 2018

14.0.0-rc.1 #1364

Merged

mjmahone mentioned this pull request Jun 7, 2018

Merge extendType functionality changes from #1322 #1373

Merged

mjmahone closed this Jun 7, 2018

snyk-bot mentioned this pull request Apr 10, 2021

[Snyk] Upgrade chai from 4.2.0 to 4.3.4 turkdevops/graphql-js#133

Closed

This was referenced Jul 1, 2021

[Snyk] Upgrade cspell from 5.2.4 to 5.6.1 turkdevops/graphql-js#234

Closed

[Snyk] Upgrade cspell from 5.2.4 to 5.6.2 turkdevops/graphql-js#237

Closed

[Snyk] Upgrade cspell from 5.2.4 to 5.6.3 turkdevops/graphql-js#239

Closed

	const oldFieldMap = type.getFields();
	Object.keys(oldFieldMap).forEach(fieldName => {

	export type GraphQLArgumentConfig = {
	type: GraphQLInputType,
	defaultValue?: mixed,
	description?: ?string,
	astNode?: ?InputValueDefinitionNode,
	};

Conversation

grenierdev commented Apr 22, 2018

facebook-github-bot commented Apr 22, 2018

Choose a reason for hiding this comment

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

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IvanGoncharov commented Apr 22, 2018

facebook-github-bot commented Apr 23, 2018

grenierdev commented Apr 26, 2018

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

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IvanGoncharov commented Apr 26, 2018

IvanGoncharov commented Apr 26, 2018

Choose a reason for hiding this comment

IvanGoncharov commented Apr 27, 2018

grenierdev commented May 10, 2018

jgcmarins commented May 10, 2018

IvanGoncharov commented May 10, 2018

Choose a reason for hiding this comment

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

Uh oh!

Choose a reason for hiding this comment

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

Uh oh!

Choose a reason for hiding this comment

IvanGoncharov commented May 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Uh oh!

Choose a reason for hiding this comment

IvanGoncharov commented May 24, 2018

grenierdev commented Jun 6, 2018

mjmahone commented Jun 7, 2018

Labels

5 participants

IvanGoncharov Apr 22, 2018 •

edited

Loading

IvanGoncharov Apr 26, 2018 •

edited

Loading

IvanGoncharov May 10, 2018 •

edited

Loading

IvanGoncharov May 10, 2018 •

edited

Loading

IvanGoncharov May 10, 2018 •

edited

Loading