Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add proper type inference by way of Subscription and SubscriptionWithHandler. #82

Merged
merged 1 commit into from
Jul 1, 2019

Conversation

parkerziegler
Copy link
Contributor

This PR would close #70 and #72 and, in my opinion, should be the feature cap for v1 🎉 After @gugahoa's awesome GADT approach for useSubscription, I was really excited to port it over to the Subscription component. However, as this issue demonstrates (thanks for opening @gugahoa) the @react.component PPX has no idea what to do w/ locally abstract types. Bummer.

I don't expect to see a fix for that in the near future, so, in the interest of getting v1 out, I decided to expand the API surface to include Subscription and SubscriptionWithHandler. The former will infer Data(d) as the parsed result of the subscription, while the latter will infer it as the accumulator 'acc type inferred through use of the handler prop. I hate that we have to increase API surface to get proper type inference, but I also think it's small enough (and likely not used a ton in comparison to the hook) that I can live with it.

I added an example of using SubscriptionWithHandler to our subscription example to demonstrate usage. I had to create a new subscription type and resolver on the server since subscription-transport-ws has memory leek issues with multiple clients using the same subscription: apollographql/subscriptions-transport-ws#433

But hey, now we have some rad abstract art 😂

cc/ @Schmavery @gugahoa

@parkerziegler parkerziegler requested a review from kiraarghy June 29, 2019 20:54
Copy link
Contributor

@gugahoa gugahoa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

@parkerziegler parkerziegler merged commit d6813f9 into master Jul 1, 2019
@parkerziegler parkerziegler deleted the task/type-inference-gadt-for-subscription branch July 1, 2019 01:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement better type inference in components.
2 participants