Graphql-js: Road to Typescript

[IvanGoncharov]

This is an issue is to discuss our plan to convert this library to TypeScript.
Note: We will still continue to provide full Flow typings in the same package.

This effort is in line with the ongoing conversion of GraphQL IDE packages (GraphiQL, GraphQL LSP, etc.): https://github.com/graphql/graphiql/tree/convert-to-ts

Note this decision is not related to the technical aspect of Flow vs TS.
GraphQL.js has complete Flow coverage and it is excellent for ongoing maintenance. But our goal is to increase the number of active contributors and by switching codebase to TypeScript we hope to make the codebase feel more familiar and easy to contribute to.

Big thanks to @JacksonKearl from Apollo team for making PoC for such conversion and exploring different conversion strategies.

Before we start discussing the action plan here is a few important points:

• 14.x.x is stable release so we shouldn't do any potentially breaking things in this release.
• We need to do 15.0.0 anyway since it becomes hard to maintain support for Node6 since ESLint and other projects stop supporting it. Plus it allows us simply remove all deprecated stuff that was scheduled for removal in this release.
• Even after we release 15.0.0 we will still need to support 14.x.x as a branch and backport all critical or security-related bugfixes until 16.0.0.

Giving these constraints here is a plan I want to propose:

• [x] 1. Freeze all non-critical merges for the duration of this conversion
• [x] 2. I copied all *.d.ts from @types/graphql into tstypes folder and working on removing it from DefinetlyTyped. I included this typings in 14.5.0 release as the baseline for our future work.
• [x] 3. Synchronize *.d.ts files with Flow types (some stuff is missing and a lot of types are based on 14.0.0 release) and identify all changes that were done in @types/graphql but weren't reflected in Flow. We will release these changes as 14.5.1 release.
• [x] 4. After we are sure that *.d.ts files are complete and don't break existing TS libraries we can branch of 14.x.x and switch master to 15.0.0-alpha.1. Remove all stuff marked as deprecated and was scheduled for removal in v15.
• [ ] 5. We will start manually converting files one by one to TS and extract Flow typings into separate files. I propose first to convert all test files (should be pretty fast since they don't use a lot of typings) and latter switch to converting the rest of the project. We can schedule frequent 15.0.0-alpha.x releases just to get the early feedback.
• [ ] 6. After we converted all files we can switch to 15.0.0-beta.x and ask the different project to test both our TS and Flow typings. After we fix all the issues we would release 15.0.0 and finish this conversion effort.

Also, I think it makes sense use this opportunity and fully convert our codebase to ES6 and idiomatic TypeScript. It means we can finally get rid of pre-ES6 classes constructs and also some weird constructs caused by Flow technical limitations
and other similar artifacts.

CC: @kassens @Neitsch @jbaxleyiii @JacksonKearl @martijnwalraven @mjmahone @acao @19majkel94 @tgriesser @nodkz @benjie

[IvanGoncharov]

We need to decide which minimal TypeScript version we should target?
For 14.x.x we stuck with 2.6 since we inherit it from DefinetlyTyped and changing it would be a breaking change.
However, for 15.0.0 we need can update it.
I read through TypeScript changelog and we absolutely need unknown since Flow's mixed is very common in our source code.
That's mean it should 3.0 or above.

@acao @benjie What TS version are you targeting for graphql-ide?
Does anyone oppose setting it to be above 3.0?

With Flow, we were always targeting the latest release so do we need to support 3.1, 3.2, 3.3, 3.4, 3.5 or we can target upcoming 3.6 from the beginning?

[IvanGoncharov]

Just release 14.5.2 with update TS typings 🎉
Big thanks to @JacksonKearl who single-handedly syncronize 102 *.d.ts in less than two days 👏

@kassens @Neitsch @jbaxleyiii @martijnwalraven @mjmahone @acao @19majkel94 @tgriesser @nodkz @benjie Can you please update to 14.5.2 or at least try it locally and provide feedback?
Ideally, this would be the last release in 14.x.x line before we move it into the branch and switch master to 15.0.0-alpha.1. So it would be great to resolve all major issues before we do that.

[eschan]

i just tried to upgrade to 14.5.2 and getting the following build error.

node_modules/graphql/utilities/coerceInputValue.d.ts:2:30 - error TS2307: Cannot find module 'tstypes/error'.

2 import { GraphQLError } from 'tstypes/error';
                               ~~~~~~~~~~~~~~~

node_modules/graphql/validation/validate.d.ts:6:19 - error TS2307: Cannot find module 'tstypes/tsutils/Maybe'.

6 import Maybe from 'tstypes/tsutils/Maybe';
                    ~~~~~~~~~~~~~~~~~~~~~~~

[JacksonKearl]

@eschan I’ll take a look at that, thanks!

[JacksonKearl]

@eschan This should be fixed with #2120.

[IvanGoncharov]

@eschan @JacksonKearl released 14.5.3 📦 with this fix.

[eschan]

works! 💯

[Neitsch]

Regarding step 5: What are the thoughts on converting everything by hand, as opposed to running a script that converts everything (https://github.com/bcherny/flow-to-typescript), and then cleaning up any broken tests afterwards?

[JacksonKearl]

@Neitsch I actually have branch up on the Apollo fork that does exactly that. https://github.com/apollographql/graphql-js/tree/ts-merge-all-at-once-wip

Issues experienced:

1. The converter errors out with unhelpful messages on most of the larger files (and even some small ones). Apparently some Flow constructs we were using aren't handled for whatever reason. This means you end up converting the "difficult" files by hand anyways.
2. The outputted types are still Flow-y. But this I mean they don't take advantage of any of TS's features, and we lose power Flow has that TS doesn't. So the result code is strictly worse-typed. For instance, we would lose things like sealed objects, and do not gain things like private class members (unless we were to make some crazy flow transform that converted underscored vars to private, which could be cool).
3. The Flow code is missing lots of types where Flow is able to infer them but TS cannot (local function parameters, for instance), which results in lots of errors under any strict flags, which we'd like to use. (~1500 errors in the codebase when strict: true is set)

[MichalLytek]

Can you please update to 14.5.2 or at least try it locally and provide feedback?

All works in TypeGraphQL, no TS errors 😉

[Neitsch]

@JacksonKearl thank you for the quick response. Makes sense! 👍

@IvanGoncharov will do the bump this week and report back

[acao]

@JacksonKearl I highly recommend @benjie's fork for flow-to-typescript. Some of these issues are addressed, amongst others. We are experimenting with using it for the IDE ecosystem, however most of the migration effort so far has been manual.
@IvanGoncharov this all sounds great and we will be following a somewhat analagous route on the IDE side. The typescript version I'm using for the monaco mode development has been latest, 3.5.3 or so.
@Neitsch and I are digging in on the LSP, we've already been underway with converting GraphiQL to typescript, and hope to release soon with our own type definitions, if not most of the packages converted. We will similarly continue to maintain the flow types, 96K downloads a week just for our flow types package!?

[yaacovCR]

This line -- I think -- causes typescript to assume that a type that fails isSpecifiedScalarType test is actually not a GraphqlScalarType at all.

https://github.com/graphql/graphql-js/blob/a482d3b24d7a6d4a8027614ab6304e980548e16f/tstypes/type/scalars.d.ts#L11

Let me know if I should open a separate issue.

[benjie]

(By the way I forked, did more work on, and released flow2typescript to npm in case you want to test it: https://www.npmjs.com/package/flow2typescript)

[Neitsch]

Apologies that it took so long. It appears to work fine :)

[mike-marcacci]

I have some _substantial_ changes to the types that provide much better safety by validating object configurations against expected types. I briefly mentioned this during the last WG meeting, and I want to make sure I get in here before my changes would be a substantial new undertaking on top of the existing TS conversion.

I can put in ~30 hours over the next two weeks into this. I'm more than willing to start from the flow code on master (possibly using a tool like flow2typescript to kick-start), but I don't want to duplicate anyone else's efforts. What's the best way for me to get involved here?

Because this touches most of the codebase I assume the best strategy is to move as quickly as possible to minimize conflicts. The features enabled by my changes are very important to a team that I'm helping start a big GraphQL project, so I have extra motivation for getting this done quickly.

[IvanGoncharov]

@mike-marcacci Thanks for the interest, we definitely need your help 👍
In the next couple of days, I will try to merge all remain PRs for 15.0.0 so we can do a feature freeze and merge #2139. After that, we can start manual conversion starting from tests.

I have some substantial changes to the types that provide much better safety by validating object configurations against expected types.

Moving to TS is already a big change and we also have a commitment to keeping Flow typings in sync. If we start substantially changing types that will be hard for both TS and Flow clients to migrate.

But I'm totally open to merging any fixes/improvements to existing types if we can keep Flow and TS in sync and before we started doing the conversion since after we convert tests to TS we will lose the ability to tests Flow typings.

At the same time, we will need to release 16.0.0 in January to drop Node v8 support so we can plan it for this release. This time we can prepare in advance and have an early experimental branch and 16.0.0-alpha.x releases just after we release 15.0.0.

@mike-marcacci Just to keep this discussion going in parallel can you please open a separate issue and give some examples of code changes you want to make?

[mike-marcacci]

@IvanGoncharov this sounds like the perfect strategy, and I will get started on a draft PR as soon as #2139 gets merged! I've opened #2188 to track this.

[yaacovCR]

Wondering about integration with https://github.com/sikanhe/gqtx

[mike-marcacci]

Hi @yaacovCR, I definitely can't speak for this project in its entirety, but I doubt that there is a desire to change the API of this reference implementation that substantially.

However, the goal behind the library you linked (strict, generator-free type safety using TypeScript) is the same goal I have for GraphQL v16. My strategy is outlined in #2188, and I plan to start the conversion as soon as is practical.

@IvanGoncharov just to touch base again, I know we have some other initiatives already in progress for defer/stream; would it be helpful for me to start a PR for the typescript conversion earlier, before those are finalized?

[pranshuchittora]

1. We will start manually converting files one by one to TS and extract Flow typings into separate files. I propose first to convert all test files (should be pretty fast since they don't use a lot of typings) and latter switch to converting the rest of the project. We can schedule frequent 15.0.0-alpha.x releases just to get the early feedback.

@IvanGoncharov I am trying to migrate the tests to TS. It would be great if we can migrate the testing lib to jest. (P.S. the current setup uses chai and mocha).

Why change the testing library?
Let's not talk about the features & differences. Rather to keep all the projects on the same page as 'graphiql' also uses jest + TS setup.

[benjie]

I foresee https://github.com/graphql/graphql-js/blob/3f859d4e5e6088deea97193d5106382a901a9c21/src/type/definition.js#L705 as being a potential issue during conversion. If we're letting systems add their own fields to this, it would be best if it was done in such a way that type safety wasn't lost for those systems.

One potential solution to this is to use an interface (one per type, e.g. GraphQLObjectTypeExtensions) for this attribute type and then allow extending the interface via declaration merging.

Does anyone know a better approach?

[MichalLytek]

@benjie allow extending the interface sounds like a good approach, it is common in Express 👍

[benjie]

Raised in #2465

[dotansimha]

@IvanGoncharov I followed your plan and migrated all tests files to TS: https://github.com/graphql/graphql-js/pull/2609 :)

[tvvignesh]

Hi. While unrelated to this issue, faced a typings issue where the code worked right but typescript threw error when I tried to access the name of a query operation like this: query.definitions[0].selectionSet.selections[0].name.value; where query is of type DocumentNode

Have currently added a @ts-ignore to ignore this and it works. Just thought of sharing here.

[mike-marcacci]

Hi all - I'm just checking in on the progress here. Is #2609 the current state of this initiative? It's possible I will be able to get time in the near future to dedicate to an implementation of #2188 (more complex, safer types). However, this is a massive undertaking that will require real coordination in this project, given that almost any concurrent changes will result in merge conflicts.

I originally anticipated that the codebase would be converted to TS by others using roughly the same types as used in the current .d.ts files, and that I would then refactor these into the more complex and powerful types I desire. However, given the apparent pace of the initial step, I suspect it may be more pragmatic to simply do the conversion all at once.

If I can get the time for this (which will probably have to come from both "work" and "personal" hours), I would prefer to ignore the issue of flow typing, and let someone else focus on that. My goal would be to introduce as few runtime changes as possible (ideally zero), which would allow the existing public flow types to remain valid.

Because the configured argument and resolver types do not propagate between the instances of the classes in this library, we have found the experience to be very frustrating and confusing for members of my current company who are either new to GraphQL or new to TypeScript. Given this and my familiarity with the problem and codebase, I think it's possible to find the hours to make this happen. However, I have zero interest in maintaining a fork, so it would have to be done in cooperation with the other folks here.

Anybody have any thoughts on this?

[wtrocki]

I recently had great success on automated conversion using flowToTs tool. More info about this here:

https://skovhus.github.io/blog/flow-to-typescript-migration/

[Urigo]

Hi @mike-marcacci this is awesome!

We've had our first working group meeting 10 days ago.
You can watch it here.
Here is a list of the action items we decided on.

One of the tasks there is Ivan to create detailed separate issues for the Typescript migration so that contributors could follow and help - @IvanGoncharov could you update about the status of this one?
I guess that's the most important one in the context of the current issue in order for us, @dotansimha @mike-marcacci and others to know where could we make progress and help you.

In the meantime I'll try to write what I understood about the current situation until you create those lists of issues/tasks:

From what I can see now, migrating the tests was the next task you asked for.
I believe @dotansimha did that here: https://github.com/graphql/graphql-js/pull/2609

From looking at the thread, you've started reviewing it, and @dotansimha was fixing the comments you were making.
Then you asked to separate some changes into separate, smaller PRs.
So @dotansimha created https://github.com/graphql/graphql-js/pull/2616 and https://github.com/graphql/graphql-js/pull/2634 got comments from you on them and fixed your comments on those as well but none of them got merged.

If I understand correctly, we have 3 open PRs that are waiting for further comments or merging and there is no open task that @dotansimha is aware of in order to continue the progress?

But I might be reading the history wrong so let's wait for your update so we, @dotansimha, @mike-marcacci and others who want to push this forward would know what needs to be done.

[mike-marcacci]

Thanks for the update @Urigo - I haven't been able to make the spec WG meetings for quite some time, and didn't realized that a separate group was spun out for graphql-js. It appears that repos/etc for the new WG are still pending creation, but I'll make sure to subscribe to those once they exist.

I'll definitely wait on those existing test PRs to be merged, and I look forward to @IvanGoncharov's other concrete issues (or a discussion of how we may all want to approach this).

@tvvignesh thanks for the note about that codemod! That would be a perfect starting point.

[ephemer]

@mike-marcacci I have been working on this the last week and have a shim over the plain graphql that strongly types pretty much everything I can think of. It's about 150 lines of (almost exclusively TypeScript) code + comments .

Ideally I would like to get this functionality into the plain graphql repo too, so I am happy to share thoughts and techniques on this if anyone finds it useful.

[mike-marcacci]

@ephemer this is awesome! Do you have your changes in a repo somewhere? Did you implement this by changing the public .d.ts files or as part of a codebase conversion? Either way, I'd love to see your implementation.

I think the current holdup is just coordination within the project. I'm hoping to hop onto the next graphql-js specific WG meeting and advocate for this, but I still don't see a repo tracking those agendas? @IvanGoncharov would you be able to point me in the right direction if I'm missing something here?

[ephemer]

@ephemer this is awesome! Do you have your changes in a repo somewhere?

Not yet - if I have time I will publish these somewhere this week.

Did you implement this by changing the public .d.ts files or as part of a codebase conversion?

I didn't change anything, I started going down the path of altering the existing types but it seemed like more work to do it that way, at least to start with.

In the end I just added some helper functions that look like:

function ResolvedField(input) {
    return input
}

but adding types to input (compatible with the existing ones) to ensure the args, type, and resolve conform to one another.

It should definitely be portable to the existing code base without too much effort. It would really help to have at least things like Scalars and the wrapping types (List and NonNull) written in real typescript though. The issue is the wrapping types both look the same to TS, as is; all the scalars too.

I will let you know when I get it online. In the meantime we are testing it internally and will continue to make improvements if needed.

[joostheijkoop]

Hello All,

I love to help out where I can. I'm Quite a GraphQL n00b, but I'm quite proficient with JS, TypeScript, Scala and Kotlin. I can do quite a bit of type wrangling and I'm really interested in GraphQL ;)

I might be able to convert something, help out with / comment on https://github.com/graphql/graphql-js/pull/2609 or pair with someone (if we can somehow set this up)

Just shout!