[@types/graphql] Add generic type on ExecutionResult#26763
[@types/graphql] Add generic type on ExecutionResult#26763mhegazy merged 3 commits intoDefinitelyTyped:masterfrom
Conversation
|
@clayne11 Thank you for submitting this PR! 🔔 @TonyPythoneer @calebmer @intellix @firede @kepennar @freiksenet @IvanGoncharov @DxCx @rportugal @tgriesser @dyst5422 @adnsio @divyenduz @bradzacher - please review this PR in the next few days. Be sure to explicitly select If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead. |
typescript-bot
added
Popular package
|
IMHO, as the definition of |
|
All this does is add a generic type. I can default it back to the previous definition if you prefer? export interface ExecutionResult<TData = { [key: string]: any };> {
errors?: ReadonlyArray<GraphQLError>;
data?: Readonly<TData>;
}Would you prefer that? Then there's effectively no change but we have the ability to add a generic. |
|
@clayne11 I agree with @firede we shouldn't deviate from |
bradzacher
left a comment
bradzacher
left a comment
There was a problem hiding this comment.
You should probably add a default generic value to all of these new definitions so that there is no risk of this being a breaking change.
| export interface ExecutionResult<TData> { | ||
| errors?: ReadonlyArray<GraphQLError>; | ||
| data?: { [key: string]: any }; | ||
| data?: Readonly<TData>; |
There was a problem hiding this comment.
Readonly will probably cause some bugs for people attempting to consume + transform the data after execution.
Probably just make it TData here instead to reduce the risk of a breaking change.
typescript-bot
added
Revision needed
|
@clayne11 One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits or comments. Thank you! |
typescript-bot
added
the
Revision needed
Without a generic type on ExecutionResult we can't have typesafe operations.
clayne11
force-pushed
the
add-fetch-result-generic
branch
from
a822ba3 to
46ffe35
Compare
typescript-bot
added
the
Revision needed
|
🔔 @bradzacher - Thanks for your review of this PR! Can you please look at the new code and update your review status if appropriate? |
typescript-bot
added
Revision needed
Reverts DefinitelyTyped/DefinitelyTyped#26763 Since we can't gurantee that response match types it was basically glorified type cast.
Reverts DefinitelyTyped/DefinitelyTyped#26763 Since we can't gurantee that response match types it was basically glorified type cast.
6 participants
Without a generic type on ExecutionResult we can't have typesafe
operations.
If changing an existing definition:
tslint.jsoncontaining{ "extends": "dtslint/dt.json" }.