[WIP/Discussion] Enhance types #394
Conversation
|
@ianks: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/ |
| let name = operationOptions.name; | ||
| const newResult = { [name]: result, ownProps: this.props }; | ||
| // Prevents us inferring useful type information :/ | ||
| if (mapResultToProps) return mapResultToProps<typeof newResult, { [i: string]: any }>(newResult); |
There was a problem hiding this comment.
This is currently preventing us from inferring the type of the result props since the user can feasibly map them to whatever they desire. Perhaps there is a niftier way to do this?
| if (mapResultToProps) return mapResultToProps<typeof newResult, { [i: string]: any }>(newResult); | ||
|
|
||
| return { [name]: defaultMapResultToProps(result) }; | ||
| } else if (this.type === DocumentType.Mutation) { |
There was a problem hiding this comment.
This is another cash that prevents us from deducing the type information easily. Since the DocumentType is determined at runtime, we have to dynamically assign the key as either mutate or data. There is potentially a way to solve this via Type Guards, but I am not 100% sure it will work as expected.
Another (possibly better in the long term) solution would be to separate the graphql function into two functions: query and mutate. This way, a lot less work is spent determining things at runtime; and we can make better guarantees about the types.
src/graphql.tsx
Outdated
|
|
||
| return GraphQL as React.ComponentClass<T>; |
There was a problem hiding this comment.
Two issues:
-
Since we currently cannot infer the types of the props that are decorated, I am simply using the type of the wrapped component here. Obviously, this gives us nothing w/r/t the types that are passed in; but as it stands the user was not getting any of this information to begin with.
-
I cannot return
GraphQLdirectly since the type is not exported. And I do not know how to export the type 😕
|
|
||
| return GraphQL as typeof WrappedComponent; |
There was a problem hiding this comment.
Two issues:
-
Since we currently cannot infer the types of the props that are decorated, I am simply using the type of the wrapped component here. Obviously, this gives us nothing w/r/t the types that are passed in; but as it stands the user was not getting any of this information to begin with.
-
I cannot return GraphQL directly since the type is not exported. And I do not know how to export the type 😕
|
@ianks thanks for the PR! I'll let @jbaxleyiii decide how much refactoring he wants 😁 |
|
Sounds good @helfer. Fortunately, this refactoring does not affect business logic. Just enhances the type information so that in the future, this lib can exist more harmoniously with Typescript setups. 😄 |
|
@ianks sorry for the long silence, I was out on paternity leave. I am 100% for getting better types with react-apollo. TBH I've even been considering converting the project to flow since that is what we use at NewSpring. Finding a way to have both strong flow and ts types is critical to me and I welcome your improvements! @johnthepink do you have any interest in helping me write some flow bindings? I'm wondering if we can convert the project to flow, then manually export ts? The reason I ask is because ts has a way to bundle types with a library, whereas flow does not currently (that I'm aware of). So if we wrote it with flow, and included ts types. All users could benefit without downloading extra types via definitely typed / npm / flow-typed. |
|
@jbaxleyiii happy to help if i can |
|
I’d be comfortable with converting to Flow types. My current position on Flow vs. TypeScript (which is constantly evolving) is that Flow is better for frontend codebases and TypeScript is better for backend codebases. The reason for this being is that Flow integrates well with critical frontend build tooling like Babel. In addition, Flow never ever transforms code. It only drops types. So the code you write with flow is exactly the code you end up running in the browser minus types. TypeScript is better for the backend because Babel integration and bundling is not as important, in addition the VSCode tooling is much more valuable for a backend developer. Frontend React codebases are mostly characterized by a bunch of leaf component files which are mostly independent of each other whereas backend codebases can have many files that are very interdependent. However, I don’t think now is the right time to switch to Flow. Switching to Flow may not even be the right move for Apollo specifically given that all our tooling is in TypeScript at the moment. If we were ever to do some major refactors of |
|
@calebmer I set up all the typescript tooling for Apollo initially ;). I dont think that all the projects having the same tooling is necessary in fact. The angular community is very heavy typescript, so that project should be in typescript. The react (and GraphQL) ecosystem is much more invested in flow. As long as react apollo maintains Ts support and adds flow support, the internal tooling doesn't matter. The issue at hand is the ease of iteration and shipping of definitions in my mind. Having both flow and TS bundled in the npm lib is a huge win and helps keep types accurate across versions. Currently once flow defs exist for the library, we will have to maintain PRs after publish to flow typed which could end up with downtime between correct types. It's why we went with ts in the first place. |
|
Personally I think converting to flow would be a mistake. 1) it's a large undertaking which does not yield a large reward 2) as far as js type systems go, typescript has a more active community and is progressing at a faster rate 3) Typescript integrates will fronted bundling tools just as well as Flow, in fact you can target ES7 which effective just strips the types and then you are free to use babel. There would have to be some pretty big wins to want to move from TS to flow, so I do not think it's justified in this case. |
|
@ianks are you still interested in improving these types? I'd be happy to help update the PR / review them now that I have some time to help with apollo again! |
|
@jbaxleyiii Yes I would. This PR has been stagnant for awhile now though. How much do you think would have to be done to get this merged up? |
|
@ianks awesome! I'm back to maintaining react-apollo more in depth and would love to get this going. How about I rebase and update the work you did and we can discuss how to solve the issues you mentioned and type anything else needed! I'll ping you when its ready! |
|
@jbaxleyiii sounds great! |
|
@ianks I started to take a look at this code more indepth, with the merge of #383 I'm not sure how to go about merging the two. Would you be able to shed some direction / help out here? Also, could you comment the typings API you would want to see built out? I'm curious to see how you would like to defined result data / props reducer shape so that the wrapped component knows what data will be coming in. Thanks! |
#383 is a manual approach to specifying the types of the injected props. Ideally, we would not have to manually maintain this interface. This PR is an attempt to more accurately specify the types within the library itself, so we do not have to manually maintain the types. This prevents type inconsistencies, allows for better maintainability of the source code, and leverages all the power of TS. We may have to include the typings of #383 for the time being, but eventually I think it would be ideal to have the library typed such that the InjectedGraphQLProps are automatically inferred. Hopefully that makes sense... I can explain more if you need since that explanation could be confusing.
In the ideal world, it would be awesome for some compiler plugin to be able to analyze the grapql query and deduce the response type automatically by referencing the schema. However, seeing as this is error-prone, the best be can do at the moment is have the As a side note, I remember reading something this week about vscode/type-checking graphql queries. I can't seem to find it now (does anyone have a link? 😉), but maybe in the future we can use the tooling behind that to power the type inference for gql queries. The future is bright! |
|
@ianks okay thats really helpful! I'll see what I can cook up. My typescript is a bit rough but I'll give it a shot. Thanks! |
|
@ianks i think you're referencing this: https://github.com/Quramy/ts-graphql-plugin |
One issue I have found while using this library is that, although the lib is written in Typescript, the types are under-specified in many areas. For instance, I would like to know exactly which props are available for me to use at any given time. This way, my editor could tell me that
this.props.data.refetchis an available method, etc.As this PR stands, I have made some headway in getting. To this point; but I am stuck and am looking for help on a few things. I will comment below and hopefully someone smarter than me can help.
Last but certainly not least, hanks for all your hard work maintaining this repo 👍