RFC: phase-dependent typings for Builder

[phryneas]

So, I already have some collection of ad-hoc typings for Builder, and I thought of doing something useful with them.

This would expose different interfaces like BuildPropertiesIntroducedIn_GraphQLObjectType that can be extended from plugin's type definitions.
These extended types will be available in (in this case) the GraphQLObjectType phase as Partial (because these typing can't track if they've already been set within the phase, and fully in all following phases.

I also added such type augmentations for PgIntrospectionPlugin and pgBasicsPlugin, as these are the one that have most impact on my code.

As a first result, this uncovered some errors in the typings of makeExtendSchemaPlugin as the graphql types there were referenced as instance types, not their respective constructors/classes.

This has a LOT of any in it right now, but that already is better than the current state of affairs, as it at least describes the existence of keys. Where I could easily add types, I did so.

But at the moment, I'd rather wait for your input, if something like this is a direction you'd like to be going instead of charging further ahead ;)

[phryneas]

This is a good example on how most plugins would be using it to extend Build from their type defintions.

In the build phase, pgIntrospectionResultsByKind will be undefined | PgIntrospectionResultsByKind - in all later phases, it will be PgIntrospectionResultsByKind.
In earlier phases (okay, there's nothing earlier than build, but you get the gist), the property wouldn't be present.

[benjie]

I don't think List or NonNull should be here?

[benjie]

This seems out of place?

[benjie]

This should be the class type: GraphQLConstructorType or whatever.

Suggested change

	addType(type: GraphQLNamedType, origin?: string): void;
	addType(type: GraphQLConstructorType, origin?: string): void;

[benjie]

Interesting concept!

Once the build hook is complete, the Build object gets frozen so it cannot be augmented further (only the object itself is frozen, children can still change) so you only need two objects really: base Build and the Build after the build hook has completed.

It'd be interesting knowing which things you can/cannot rely on in the build and inflection hooks, but alas I don't think we can actually implement it, since the build object passed to each build hook will differ based on the previous hooks that have executed and these run in a user-configurable (and augmentable) order. So I worry this is not returning sufficient value to justify the increased complexity/maintenance costs. I think it might be better solved by making the Build type into BaseBuild & Partial<BuildExtensions> for the build and inflection hooks which are called before Build is finalised, and BaseBuild & BuildExtensions for those thereafter. Plugins should then extend BuildExtensions.

My plan is to use "declaration merging" in the TypeScript conversion to model these objects a lot better; you can read more about that here: #553 (comment)

I'd advise that you don't invest too much time in our TypeScript typings right now unless you can apply it to our conversion process.

I'm going to close this for now (as mentioned in the above comment I'm trying to close all the PRs before moving to TypeScript), but please raise any small typing fixes in their own PRs (for example your change to makeExtendSchemaPlugin).

Thanks again for putting this time in; I hadn't considered the BaseBuild & Partial<BuildExtensions> approach before but I think it'll improve typings significantly (though it will probably cause some churn for users who have TypeScript plugins!).

[phryneas]

Once the build hook is complete, the Build object gets frozen so it cannot be augmented further (only the object itself is frozen, children can still change) so you only need two objects really: base Build and the Build after the build hook has completed.

Ah, that was the part of information I was missing. I had seen some plugins that changed stuff on build in later phases, but now that I think about it - they changed just children.

It'd be interesting knowing which things you can/cannot rely on in the build and inflection hooks, but alas I don't think we can actually implement it, since the build object passed to each build hook will differ based on the previous hooks that have executed and these run in a user-configurable (and augmentable) order. So I worry this is not returning sufficient value to justify the increased complexity/maintenance costs.

Yeah, I had thought about actually implementing something like that, but given that the order is determined dynamically just by "before" and "after" references on other plugins, there's no way that could be implemented in TypeScript. I guess that's for the better :D

I think it might be better solved by making the Build type into BaseBuild & Partial<BuildExtensions> for the build and inflection hooks which are called before Build is finalised, and BaseBuild & BuildExtensions for those thereafter. Plugins should then extend BuildExtensions.

Sounds like a great plan!

I'm going to close this for now (as mentioned in the above comment I'm trying to close all the PRs before moving to TypeScript), but please raise any small typing fixes in their own PRs (for example your change to makeExtendSchemaPlugin).

Will do, probably during my next slacktime or so 👍

I'm really looking forward to that TS transistion :)

[benjie]

Will do, probably during my next slacktime or so +1

Awesome 🙌

I'm really looking forward to that TS transistion :)

You and me both 😅