Change type of extensions from anonymous Record to named interfaces #2465

benjie · 2020-02-28T16:05:32Z

Following on from #2104 (comment) I thought it best to demonstrate with a PR.

I'm concerned about losing type safety of the extensions field due to the any's here. I consulted @orta on the best way to type this in TypeScript, and he confirmed that using an empty interface and declaration merging is the best practice here. He provided the following example:

https://www.typescriptlang.org/play/?ssl=12&ssc=1&pln=16&pc=1#code/JYOwLgpgTgZghgYwgAgKIA9IgM7APY7IDeAsAFDkC+55E6ADnlGMggDZzbbIDiUc9ABYBFADIB5AEYArCAjAAVAJ70UpMsmR0suAtgBcaTBBz4cNDaz1goAV3lMAFAEpiycps1hBwbADptE11CAF5iaksIiPJQSFhEFAwdM251TQBbJQA1OCgAfkNsG1AAcypyIA

He noted that the fields added to the interface by merged declarations should contain ? to avoid issues at the internal callsite, but I don't think that's an issue for us since all extension attributes would be optional; accessed via something like: myGraphQLObjectType?.extensions?.myAttribute.

~~This would be a breaking change to the types (any -> unknown), so I think it's important to include it before GraphQL.js v15 is released.~~

IvanGoncharov · 2020-02-28T16:29:53Z

@benjie Thanks for PR 👍
The big problem here is that schemas from different sources can have different extensions so we can't pollute global types.
Also, it will make it impossible to generate extensions from introspection in the future.

IvanGoncharov · 2020-02-28T16:42:55Z

@benjie Can you please describe your use case in more detail so I can think about alternative solutions?

benjie · 2020-02-28T16:52:06Z

The big problem here is that schemas from different sources can have different extensions so we can't pollute global types.

That's what declaration merging solves; everyone's type completion would be relevant to their project solely based on the dependencies they pull in; e.g.

In MyType.ts:

import { GraphQLObjectType } from "graphql";

declare module "graphql" {
  interface GraphQLObjectTypeExtensions {
    /*
     * This attribute declaration gets merged in with all the other
     * attribute declarations from every other module _that you have
     * imported_ that declares this interface.
     */
    myNumber?: number;
  }
}

export default new GraphQLObjectType({
  name: "MyType",
  fields: {...},
  extensions: {
    myNumber: 42,
  },
});

In SomeOtherFile.ts:

import { GraphQLSchema } from "graphql";
import MyType from "./MyType";
import DifferentType from "some-other-module";

console.log(MyType.extensions?.myNumber); // 42
console.log(DifferentType.extensions?.myNumber); // undefined

^ The above would not throw typing errors, and would know what myNumber was number | undefined and would know what attributes are likely to be available in extensions of a GraphQLObjectType.

The main issue this would open is that once an attribute is defined on extensions, all attributes with that name on that type of extension (e.g. GraphQLObjectTypeExtensions) would have to have the same type. This is probably a good thing; I'd suggest using namespacing to help avoid conflicts from different modules. If you want to introspect over the properties of extensions then having the attributes have consistent types is probably important anyway.

Also, it will make it impossible to generate extensions from introspection in the future.

I'd need to understand more what you have in mind here to comment; I assume it relates to graphql/graphql-spec#300 ? I don't currently see any issues here that using Record<string, any> would solve - you can just cast the type to Record<string, any> internally.

IvanGoncharov · 2020-02-28T17:02:32Z

I'd need to understand more what you have in mind here to comment;

@benjie I mean that when you generate extensions dynamically you can't guarantee that they will match any type other than Record<string, any> and it creates false sense of security.
A long term plan is to actually switch to Record<string, unknown> so it will match current Flow types:

graphql-js/src/type/definition.js

Line 574 in 3f859d4

extensions: ?ReadOnlyObjMap<mixed>;

Can you please describe your use case in more detail so I can think about alternative solutions?

benjie · 2020-02-28T17:06:23Z

I mean that when you generate extensions dynamically you can't guarantee that they will match any type other than Record<string, any> and it creates false sense of security.

You can still add the loss-of-type-safety necessary for that with declaration merging; e.g.:

https://www.typescriptlang.org/play/?ssl=1&ssc=1&pln=25&pc=1#code/JYOwLgpgTgZghgYwgAgKIA9IgM7APY7IDeAsAFDkC+55E6ADnlGMggDZzbbIDiUc9ABYBFADIB5AEYArCAjAAVAJ70UpMsmR0suAtgBcaTBBz4cNDaz1goAV3lMAFAEpiycps1hBwbADptE11CAF5iaksIiPJQSFhEFAwdM251TQBbJQA1OCgAfkNsG1AAcyoLWOh4JCNkvWIPZABtAGsIJULikBKAXUM4ECUAbnKKMgRrZDxkMJAIAHdefiExKVl5ZVUXEbG8AONTPTy-TJyoHb3Aw5xjmDw8HaA

interface Extensions {

}

export class GraphQLObjectType {
  extensions: Extensions

  constructor() { 
    this.extensions = {}
  }
}

interface Extensions {
  myVar?: string
}

interface Extensions {
  [key: string]: any;
}

const o = new GraphQLObjectType();

o.extensions?.myVar;
o.extensions?.foo;

Can you please describe your use case in more detail so I can think about alternative solutions?

The main motivation here is to avoid loss of type safety - currently as soon as you read an attribute from thing.extensions it's an any. In PostGraphile we maintain internal maps from GraphQL type instances to values we use (e.g. "look-ahead" data for efficient SQL execution, etc); it would be great to bake these into the schema directly on the fields/arguments/types/etc so that they can be accessed directly from GraphQL resolvers without requiring pulling data from a closure or demanding that context or rootValue has all this information passed in. This'd allow us to make use of .toConfig() -> save -> load and make user's lives easier.

benjie · 2020-02-28T17:09:30Z

@IvanGoncharov Would you be open to me changing these to:

export interface GraphQLObjectTypeExtensions {
  [key: string]: unknown;
}

This way you still get basically the same as you have currently (except a named interface rather than anonymous Record), but TypeScript can tab-complete and understand the declaration-merged declared attributes if people opt-into that.

MichalLytek · 2020-02-29T14:31:22Z

Can you please describe your use case in more details

The simplest use case would be query complexity:
https://github.com/slicknode/graphql-query-complexity

When we put some metadata to type's extensions property that are read in runtime and used to calculate query complexity.

So if you install graphql-query-complexity, thanks to the declarations merging it would add type info to GraphQLObjectTypeExtensions so it will provide intellisense that extensions has complexity property and prevent passing there wrong values like string:

const Post = new GraphQLObjectType({
  name: 'Post',
  fields: () => ({
    title: { type: GraphQLString },
    text: {
      type: GraphQLString,
      extensions: {
        complexity: 5
      },
    },
  }),
});

Or provide intellisense (type-inference) for the callback calculator parameters:

const Query = new GraphQLObjectType({
  name: 'Query',
  fields: () => ({
    posts: {
      type: new GraphQLList(Post),
      args: {
        count: {
          type: GraphQLInt,
          defaultValue: 10
        }
      },
      extensions: {
        complexity: ({args, childComplexity}) => childComplexity * args.count,
      },
    },
  }),
});

benjie · 2020-03-11T12:32:35Z

I've added this change. I think this is now a fairly non-controversion change:

We've given names to the previously anonymous record types (basically equivalent)
We're using an interface with index signature rather than a record (basically equivalent)
We've changed "any" to "unknown" so that types must be checked (you mentioned this was a long-term goal anyway)

benjie · 2020-03-11T15:48:51Z

To make this even more uncontroversial I've converted the unknowns back to anys. So now we're just changing anonymous record types to named and exported interface types. This code is 100% backwards compatible.

src/type/index.d.ts

src/type/definition.d.ts

IvanGoncharov · 2020-06-02T20:40:43Z

@benjie You also need to apply the same changes to GraphQLSchema and GraphQLDirective.
Other than that and review comments that left I don't have any objections.

That said I still think extending types globally is not a good idea but I don't want to block this PR since I don't object changes themself I just object how they would be used.

IvanGoncharov · 2020-06-03T13:48:43Z

@benjie Just to capture note from our chat:
Descriptions should mentioning that it's highly recommended to create wrapper object for stuff that you put inside extension to prevent clashes:

new GraphQLObjectType({
  name: 'Foo',
  // ...
  extensions: {
    apolloFederation: {
      keys: [/* <Federations specific keys> */],
    },
    postgraphile: {
      keys: [/* <Postrgress specific keys> */],
    }
  }
}

benjie · 2020-06-03T16:51:28Z

I've extended to GraphQLSchema and GraphQLDirective; exposed through src/index.d.ts, mirrored the changes to Flow, and added the following comment above the extensions in the TypeScript typings:

/**
 * It's strongly recommended that you prefix each added attribute's name with an
 * identifier unique to your project to avoid conflicts with other projects.
 */

This allows for using a namespaced wrapper object as you suggest, or just having a mess of attributes that shouldn't conflict (which may be more convenient for other projects).

There's a small issue with the Flow typings: previously you were using ReadOnlyObjMapLike for the extensions input, and then converting that to ReadOnlyObjMap on the constructed types. Since we're now sharing a single named type between these places, it's not clear how or if you want me to make this distinction. Maybe we should just use ReadOnlyObjMapLike everywhere? Or just drop the null prototype requirement?

airhorns · 2020-06-19T18:04:24Z

Would you folks be open to making the Extensions types generic over the same things the containing type is generic over so that the properties of our strongly typed extensions can be generic over those as well? If you're attaching a function as an extension to a GraphQLObjectType, it seems reasonably to want to pass that function the context some times, so to strongly type the function the extension needs to be generic over the TContext that the GraphQLObjectType is as well.

Prior to extensions being a thing at all, one library I've worked with was using TypeScript declaration merging to extend the definition GraphQLObjectType to facilitate the same thing extensions does like so: https://github.com/join-monster/join-monster/blob/master/src/index.d.ts . The optional properties it adds to those types can be thunked such that they can be functions that get the context and return a dynamic value, and that's strongly typed by extending the root type and getting a handle on the generics again. If the empty *Extensions types aren't generic, is there a way to still get a handle on the generic types?

… while we wait for GraphQL to figure out how to strongly type extensions See graphql/graphql-js#2465

IvanGoncharov · 2020-06-19T19:33:25Z

Would you folks be open to making the Extensions types generic over the same things the containing type is generic over so that the properties of our strongly typed extensions can be generic over those as well?

@airhorns Sounds reasonable the only downside I see is that unused template arguments will look strange so it we should extend their description saying they are open types and it's possible to extend them so we provide template arguments for this reason.

@benjie Can you please add simple test extending at least a couple of them in our TS integration tests:
https://github.com/graphql/graphql-js/blob/master/integrationTests/ts/index.ts
We have GraphQLObjectType there so you can extend object type, field, and argument.

IvanGoncharov · 2020-06-19T19:47:32Z

There's a small issue with the Flow typings: previously you were using ReadOnlyObjMapLike for the extensions input, and then converting that to ReadOnlyObjMap on the constructed types. Since we're now sharing a single named type between these places, it's not clear how or if you want me to make this distinction. Maybe we should just use ReadOnlyObjMapLike everywhere? Or just drop the null prototype requirement?

@benjie My initial concern was that some developers will generate extensions from some 3rd-party sources or even user-provided data in case platforms like CMS. Since the end goal is to allow exposing extensions through introspection so buildClientSchema can contract schema with extensions.
So we should be able to handle stuff like toString, toValue without constant calls to hasOwnProperty, especially since hasOwnProperty is also valid GraphQL name :(

Thinking about it just remove those types from Flow since we migrating TS in next release anyway.

IvanGoncharov · 2020-06-19T19:58:14Z

This allows for using a namespaced wrapper object as you suggest, or just having a mess of attributes that shouldn't conflict (which may be more convenient for other projects).

@benjie Can you please explain user cases that you have in mind where it's more convenient?

Because I actually prefer to standardize on one approach to not confuse library authors and their users.
Also, people like to create extremely short prefixes so for example instead of postgraphile wrapper most of the developers will try to use pg prefix which has non zero chance to conflict with any other PostgreSQL related code.

IvanGoncharov · 2020-06-19T20:04:48Z

@benjie Looking at @airhorns example it sims that join monster is currently using jm prefix that is proving my point of people get tendency for extremely short prefixes. Since the end-goal is to eventually allow to expose extensions through introspection (disabled for all fields by default) it would make sense to recommend wrapping extensions in longer names from the beginning.

benjie · 2020-06-20T06:34:24Z

@IvanGoncharov would you like to provide alternative wording for the comment?

benjie · 2020-06-20T10:49:50Z

integrationTests/ts/index.ts

+    someArgumentExtension?: SomeExtension;
+  }
+}
+*/


I don't know how best to selectively enable this for TypeScript 3.2 and above only.

Further, the types themselves are not concretely tested, pointing to an example in the GraphQL source where this takes place would be helpful.

benjie · 2020-06-20T10:54:37Z

@IvanGoncharov This is ready for re-review. All comments were addressed, however I was not able to figure out what testing could be added for the types that supports back to TypeScript 2.6. If you uncomment the block I commented, it works on TS>=3.2, but fails on 3.1:

Testing on typescript-3.9 ...
Testing on typescript-3.8 ...
Testing on typescript-3.7 ...
Testing on typescript-3.6 ...
Testing on typescript-3.5 ...
Testing on typescript-3.4 ...
Testing on typescript-3.3 ...
Testing on typescript-3.2 ...
Testing on typescript-3.1 ...
../../npmDist/index.d.ts:147:3 - error TS2484: Export declaration conflicts with exported declaration of 'GraphQLArgumentExtensions'.
147   GraphQLArgumentExtensions,
      ~~~~~~~~~~~~~~~~~~~~~~~~~
../../npmDist/index.d.ts:156:3 - error TS2484: Export declaration conflicts with exported declaration of 'GraphQLFieldExtensions'.
156   GraphQLFieldExtensions,
      ~~~~~~~~~~~~~~~~~~~~~~
../../npmDist/index.d.ts:172:3 - error TS2484: Export declaration conflicts with exported declaration of 'GraphQLObjectTypeExtensions'.
172   GraphQLObjectTypeExtensions,
      ~~~~~~~~~~~~~~~~~~~~~~~~~~~

Since this is user code, this should not be a concern (we just shouldn't add this block of code in TypeScript 3.1 or below). I considered changing index.ts to index.ts.template and then re-writing it before each test, but that seems a bad approach. I considered moving from running tsc to loading the TypeScript library programatically, but that seems expensive (loading so many versions, never unloading any). I considered doing this but in forked node processes, but this started to look complex. Perhaps you could demonstrate how you'd like this tested?

benjie · 2020-06-24T09:10:55Z

integrationTests/ts/index.ts

@@ -1,30 +0,0 @@
-import { GraphQLString, GraphQLSchema, GraphQLObjectType } from 'graphql/type';


This entire file was renamed to integrationTests.ts.template

benjie · 2020-06-24T09:11:24Z

integrationTests/ts/index.ts.template

+};
+
+// The following code block requires a newer version of TypeScript
+/*! >=3.2 !*/


This "magic comment" syntax is documented in test.js

benjie · 2020-06-24T09:13:07Z

integrationTests/ts/test.js

+  for (const template of templates) {
+    template.writeFileSync(version);
+  }
+


@IvanGoncharov I seek your guidance on this approach to allowing a block of code to be omitted when testing older TypeScript versions. It works; but it does mean the tests write to the filesystem, which makes me uncomfortable. It's a fairly simple implementation though, basically powered by a single regexp and the semver module.

@benjie Thanks for trying this solution.
How about just have index-3.2.ts and index.ts?
Cycle starts with index.ts since we start from newest versions first but if

fs.fileExistSync(`index-${version.replace('typescript-', '')}.ts`)

exist we will switch to this file for this and latter versions.

That was my first thought; but I realised that keeping these files in sync would be challenging, and it increases the amount of code in the tests without offering much in the way of value. It also doesn't allow for more complex ranges like >=3.2 <5 which may be required in future. "Turning off" a block of code seemed like the simplest solution from the point of view of writing the tests, at a slight cost of the test infrastructure being more complex.

I suppose the more complex ranges are enabled by having a 4.9 file and another 3.1 file; but this still seems more complex/cumbersome than just removing a block.

@benjie couple of points:

don't like having glob and semver as dependencies it means we need to update them constantly to prevent GitHub marking this repo as affected by yet another security vulnerability.

Version after next will drop all TS up to 3.7 (to support recursive types) so this is the temporary workaround to keep maintaining 15.x.x for some time and possibly backport critical fixes.

I don't expect this file to be changed frequently since we will migrate our codebase to TS we will need it just as a sanity check and for very rare occasions where we can't test stuff in our codebase like this one.

So I will likely remove this mechanism in 16.0.0 anyway so no need to make it too complex or future proof just make it to require as low maintenance as possible.

@benjie Let's not block this PR and keep it KISS just add test as a comment with FIXME and I will uncomment it in 16.0.0.
How does it sounds?

You already have a lot of dependencies on both of these modules according to package-lock.json, but I totally get where you're coming from

Great 👍

Okay.

I've reverted the commit that adds this testing support.

… versions" This reverts commit 04c837a.

airhorns · 2020-06-29T13:14:09Z

Bless both of your hearts for making this happen and sweating the details! Thanks!!

After graphql/graphql-js#2465 , we can now use TypeScript declaration merging to augment the graphql-types nice and cleanly. Woop woop!

* Switch to using GraphQL 15's extensions for join-monster config in a schema GraphQL 15 doesn't let schema authors attach arbitrary properties to schema objects anymore, so join-monster's config style has to change. There's an `extensions` property that works great for this, let's use that! * Update docs to reflect new extensions configuration setup * Bump join-monster version to indicate breaking change * Update TypeScript types to export strongly typed extension interfaces After graphql/graphql-js#2465 , we can now use TypeScript declaration merging to augment the graphql-types nice and cleanly. Woop woop! * Add a changelog entry explaining how to migrate to the new extensions format * Fix a couple broken TypeScript types for thunking and add TypeScript tests

benjie mentioned this pull request Feb 28, 2020

Road to Typescript #2104

Closed

6 tasks

IvanGoncharov added the needs exploration label Feb 28, 2020

benjie changed the title ~~Change type of extensions from Record to extendable interfaces~~ Change type of extensions from anonymous Record to named interfaces Mar 11, 2020

IvanGoncharov removed the needs exploration label Apr 2, 2020

IvanGoncharov added this to the v15.1.0 milestone Apr 2, 2020

JCMais mentioned this pull request May 3, 2020

Improve GraphQL type augmentation - Extensions field JCMais/graphql-yup-middleware#8

Closed

IvanGoncharov reviewed Jun 2, 2020

View reviewed changes

src/type/index.d.ts Show resolved Hide resolved

IvanGoncharov reviewed Jun 2, 2020

View reviewed changes

src/type/definition.d.ts Show resolved Hide resolved

IvanGoncharov added the PR: feature 🚀 requires increase of "minor" version number label Jun 2, 2020

airhorns mentioned this pull request Jun 19, 2020

GraphQL v15 config via extensions join-monster/join-monster#418

Merged

airhorns added a commit to gadget-inc/join-monster that referenced this pull request Jun 19, 2020

Update TypeScript types to export strongly typed extension interfaces…

0ac27c0

… while we wait for GraphQL to figure out how to strongly type extensions See graphql/graphql-js#2465

Change type of extensions from Record to extendable interfaces

f3fb7b1

benjie added 9 commits June 20, 2020 10:48

Add named flow typings for extensions

7e2e0fb

Also export in src/index.d.ts

8791f1e

Apply the changes to GraphQLSchema and GraphQLDirective

3efe792

Mirror to flow

17b7d6b

Add a note not to conflict with other projects

c72abfb

Make Extensions types generic where appropriate

8bd6f83

Update comment.

c609839

Why template arguments?

aa6ddc6

Revert named extension types in Flow

2dbc5ca

benjie force-pushed the ts-extensions branch from 4881a33 to 2dbc5ca Compare June 20, 2020 10:26

Add extensions to typescript integration test

20d4f8d

benjie commented Jun 20, 2020

View reviewed changes

benjie requested a review from IvanGoncharov June 20, 2020 10:54

IvanGoncharov modified the milestones: v15.1.0, 15.2.0 Jun 20, 2020

benjie commented Jun 24, 2020

View reviewed changes

benjie force-pushed the ts-extensions branch from 63ebdb4 to 3dee424 Compare June 24, 2020 09:23

Allow testing certain code blocks on only specific TypeScript versions

04c837a

benjie force-pushed the ts-extensions branch from 3dee424 to 15407be Compare June 25, 2020 17:07

Revert "Allow testing certain code blocks on only specific TypeScript…

b33e65f

… versions" This reverts commit 04c837a.

benjie force-pushed the ts-extensions branch from 15407be to b33e65f Compare June 25, 2020 17:08

IvanGoncharov merged commit 7ee63b0 into graphql:master Jun 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change type of extensions from anonymous Record to named interfaces #2465

Change type of extensions from anonymous Record to named interfaces #2465

benjie commented Feb 28, 2020 •

edited

Loading

IvanGoncharov commented Feb 28, 2020

IvanGoncharov commented Feb 28, 2020

benjie commented Feb 28, 2020

IvanGoncharov commented Feb 28, 2020

benjie commented Feb 28, 2020

benjie commented Feb 28, 2020 •

edited

Loading

MichalLytek commented Feb 29, 2020

benjie commented Mar 11, 2020 •

edited

Loading

benjie commented Mar 11, 2020

IvanGoncharov commented Jun 2, 2020 •

edited

Loading

IvanGoncharov commented Jun 3, 2020

benjie commented Jun 3, 2020

airhorns commented Jun 19, 2020

IvanGoncharov commented Jun 19, 2020

IvanGoncharov commented Jun 19, 2020

IvanGoncharov commented Jun 19, 2020

IvanGoncharov commented Jun 19, 2020

benjie commented Jun 20, 2020

benjie Jun 20, 2020

benjie commented Jun 20, 2020

benjie Jun 24, 2020

benjie Jun 24, 2020

benjie Jun 24, 2020

IvanGoncharov Jun 24, 2020 •

edited

Loading

benjie Jun 24, 2020

benjie Jun 24, 2020

IvanGoncharov Jun 24, 2020

IvanGoncharov Jun 25, 2020

benjie Jun 25, 2020

airhorns commented Jun 29, 2020

		@@ -1,30 +0,0 @@
		import { GraphQLString, GraphQLSchema, GraphQLObjectType } from 'graphql/type';

Change type of extensions from anonymous Record to named interfaces #2465

Change type of extensions from anonymous Record to named interfaces #2465

Conversation

benjie commented Feb 28, 2020 • edited Loading

IvanGoncharov commented Feb 28, 2020

IvanGoncharov commented Feb 28, 2020

benjie commented Feb 28, 2020

IvanGoncharov commented Feb 28, 2020

benjie commented Feb 28, 2020

benjie commented Feb 28, 2020 • edited Loading

MichalLytek commented Feb 29, 2020

benjie commented Mar 11, 2020 • edited Loading

benjie commented Mar 11, 2020

IvanGoncharov commented Jun 2, 2020 • edited Loading

IvanGoncharov commented Jun 3, 2020

benjie commented Jun 3, 2020

airhorns commented Jun 19, 2020

IvanGoncharov commented Jun 19, 2020

IvanGoncharov commented Jun 19, 2020

IvanGoncharov commented Jun 19, 2020

IvanGoncharov commented Jun 19, 2020

benjie commented Jun 20, 2020

Choose a reason for hiding this comment

benjie commented Jun 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IvanGoncharov Jun 24, 2020 • edited Loading

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

airhorns commented Jun 29, 2020

benjie commented Feb 28, 2020 •

edited

Loading

benjie commented Feb 28, 2020 •

edited

Loading

benjie commented Mar 11, 2020 •

edited

Loading

IvanGoncharov commented Jun 2, 2020 •

edited

Loading

IvanGoncharov Jun 24, 2020 •

edited

Loading