Wrapper method on type classes

[mohawk2]

Rather than constantly do:

listType = new GraphQLList(otherType);

it would be nicer to do:

listType = otherType.wrapList();

It would be even nicer if the other type could memoise the "list" version, which would be safe because all these objects are immutable. This PR implements that.

Not included in the PR yet, since it is not working: I am trying to do the same for a wrapNonNull method but it is giving a strange type failure. It is particularly strange to me because the same syntax and type declarations work fine on the wrapList method. This is an excerpt of the error:

       Property `type` is incompatible:
        111:       type: GraphQLString.wrapNonNull(),
                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^ GraphQLNonNull. This type is incompatible with
        673:   type: GraphQLOutputType;
                     ^^^^^^^^^^^^^^^^^ union: GraphQLScalarType | GraphQLObjectType | GraphQLInterfaceType | GraphQLUnionType | GraphQLEnumType | type application of class `GraphQLList` | type application of class `GraphQLNonNull`. See: src/type/definition.js:673

It seems to be losing the "type application" on GraphQLNonNull. It did this even when I adjusted the return type of wrapNonNull from GraphQLNonNull<*> to GraphQLNonNull<GraphQLScalarType>. Any insight into why would be greatly appreciated!

[wincent]

Rather than constantly do:

listType = new GraphQLList(otherType);

it would be nicer to do:

listType = otherType.wrapList();

It's not obvious to me what is "nicer" about it. Can you explain?

[mohawk2]

@wincent To me, it's nicer:

• syntactically, especially on types that are multiply wrapped eg with NonNull, then List, then sometimes NonNull again - no mass of nested parens

• conceptually - knowing that thanks to immutable data, there is only one copy of GraphQLString.wrapList() being used anywhere.

[IvanGoncharov]

conceptually - knowing that thanks to immutable data, there is only one copy of GraphQLString.wrapList() being used anywhere.

@mohawk2 Great feature 👍 It allows comparing types directly (type1 === type2) without a need for isEqualType function. However, you can't guarantee an absence of copies unless you forbid to construct GraphQLList/GraphQLNonNull directly.

So you can't make this assumption in your code especially if you using any 3rd-party library (e.g. graphql-relay-js) to construct your schema.

I wish it was implemented before public release of graphql-js 😭

syntactically, especially on types that are multiply wrapped eg with NonNull, then List, then sometimes NonNull again - no mass of nested parens

Alternative solution could be: wrapType('[[', GraphQLString, ']!]!').
Or ES6 variant typeTag `[[${GraphQLString}]!]!` which could be implemented using tagged template literals

[mohawk2]

@IvanGoncharov Very interesting possibilities! I am thinking less adventurously: that with the memoising wrap methods, the trivial case of === in isEqualType will be true much more often, which can only be a good thing.

By the way, only having wrapList and not wrapNonNull is due to a flow type error which I cannot figure out. I have PR-ed the commits I have made for that (along with the errors I see) on my own forked repo: mohawk2#1 and if anyone can tell me what I am doing wrong that will be most helpful.

[leebyron]

I'd much prefer that graphql-js avoid adding additional methods to types, especially for type construction. I would be happy to review a PR which provides convenience functions for constructing wrapping types. WeakMaps could be used for memoization if that's still preferred.

[leebyron]

I'm going to close this PR for repo maintenance, but if you rebase with new changes please feel free to reopen it!

[mohawk2]

@leebyron I will certainly do so (and thanks for the warning about why the close :-) ).

In particular, I am happy to switch it to a function if you require that. I would note and argue that in this specific case, there is a genuine syntactic benefit to being a method, because that makes it postfix. That makes it a good match for at least the NonNull specification in the SDL.

Before I go ahead, it would really really help if you could look at the PR I linked above (mohawk2#1) which has a Flow error for the NonNull version of this, despite looking identical to me. If it's not obvious to you almost immediately, then don't worry (but please say) and I will dig into debugging Flow/my code.

[leebyron]

I've made some improvements in the master branch both to defining non-null and list (no longer need the new keyword) and flow types which should hopefully help

[leebyron]

The flow error you're encountering is due to using the * type. Flow often cannot resolve them and they result in errors. You should avoid them whenever possible in favor of explicit types

[mohawk2]

Thanks for taking a look!

[mohawk2]

@leebyron I have updated the PR! However I don't seem to be able to re-open it, could you or someone? I added the memoising with the WeakMap per your suggestion, and after some Flow nightmares it all seems to work. It even seems (using my benchmark) to be a few percent faster!

@IvanGoncharov I switched to using your idea of wrapType, but only using the notional right-hand side, since having both sides would have made it very complicated to program, with an unnecessary possibility of errors: what if the programmer got a mismatched number of brackets? So now it looks like this:

wrapType(GraphQLInt, '!]!]!');

That seems to me very concise and expressive. Thanks!

[IvanGoncharov]

I have updated the PR! However, I don't seem to be able to re-open it

It's not because of access rights since I also see this message:

It looks like GitHub limitation: isaacs/github#361

[mohawk2]

Hi @IvanGoncharov, what a strange limitation! Thanks for the information.

Anyway, I've re-force-pushed this back to the old commit. Could you reopen it so I can force-push the new code? In meantime you can look at it on https://github.com/graphql/graphql-js/compare/master...mohawk2:wrapmethods-new?expand=1 if you want to.