Upgrade TS to 3.7.3#3618
Merged
Merged
Conversation
This commit updates our typescript version to latest. It also incorporates related fixes (workarounds?) that have blocked the upgrade since 3.6.3. I haven't yet been able to identify the underlying issue, nor do I think it's worth my time at this point. The fixes proposed here do limit some of the generic-ness of the Dispatcher class and its functions, however I'd argue that the level of said generic-ness is unnecessary. * The Dispatcher really only needs to accept a GraphQLListener * A GraphQLListener _only_ has methods for properties, so the hoops we previously jumped through to collect the names of strictly function properties on an object is unnecessary and greatly simplified by 'keyof T'. It's worth mentioning a couple things: * No type inference is lost where we consume Dispatcher. Autocomplete still smartly suggests the allowed function names of a GraphQLListener as we would hope * The types being modified don't exist in the public API, so we shouldn't have any concerns with breaking type changes to the Dispatcher class.
abernix
approved these changes
Dec 17, 2019
abernix
left a comment
abernix
left a comment
Member
There was a problem hiding this comment.
Thank you for looking into this!
abernix
added a commit
that referenced
this pull request
May 6, 2020
As I reached to introduce a dispatcher for an interface that wasn't a `GraphQLRequestListener`, it dawned on me that the dispatcher that we already had was pretty close to being correct for this prior to the changes in the linked PR/commits below ([[Ref 1]][[Ref 2]]). Within those changesets, the addition of `GrahpQLRequestListener` as a constraint to the `Dispatcher`'s `T` type was done as a matter of necessity so TypeScript could be updated to the latest version. Further analysis, with a motivation to boot, led me to believe that TypeScript was correct in its newfound determination (circa ~v3.7.3) that the `apply` method didn't exist on the keys of the interface being invoked. Concretely, I'm pretty sure there was no constraint that was actually ensuring they were functions and some degree of indirection caused TypeScript to give up trying to reconcile that. This brings back a generic property to the `Dispatcher` by using an object with `string`-key'd properties and functions as values as the constraint for the entire `Dispatcher` interface, rather than `GraphqLRequestListener` itself. Ref 1: 1169db6 Ref 2: #3618
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This commit updates our typescript version to latest. It also incorporates related fixes (read: workarounds?) that have blocked the upgrade since 3.6.3. I haven't yet been able to identify the underlying issue, but this seems like a TS regression that's over my head.
The symptom is that within each of our
invoke*functions,methodis no longer benefiting from thetypeof method === 'function'check. Within theifblock,methodis being inferred as aT[TMethodName]rather than aT[TMethodName] & Function- hencemethod.applywas now considered invalid (asmethodis no longer also aFunctionaccording to TS).The fixes proposed here do limit some of the genericness of the Dispatcher class and its functions, however I'd argue that the level of said genericness is unnecessary.
Dispatcherreally only needs to accept aGraphQLListenerGraphQLListeneronly has methods for properties, so the hoops we previously jumped through to collect the names of strictly function properties on an object seem unnecessary and are nicely simplified bykeyof T.It's worth mentioning a couple things:
Dispatcher. Autocomplete still smartly suggests the allowed function names of aGraphQLListeneras we would hope.