-
Notifications
You must be signed in to change notification settings - Fork 536
Conversation
@IvanGoncharov My current plan is to:
|
types/index.d.ts
Outdated
@@ -56,64 +58,64 @@ declare namespace graphqlHTTP { | |||
/** | |||
* A value to pass as the context to the graphql() function. | |||
*/ | |||
context?: unknown; | |||
context?: Maybe<unknown>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unknown
should include null
and undefined
right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see it marked as ?mixed
in Flow.
So this change is ok 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it can fixed in separate PR both in Flow and TS
types/index.d.ts
Outdated
|
||
/** | ||
* A boolean to configure whether the output should be pretty-printed. | ||
*/ | ||
pretty?: boolean | null; | ||
pretty?: Maybe<boolean>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing to note: after TS conversion we need to get rid of Maybe
types, AFAIK they are not part of idiomatic TS and at that point would be just a legacy from the time that we used Flow.
@danielrearden It looks like TS by default supports Moreover, it looks like Flow has the same behaviour: |
Motivation: graphql#622 (comment)
Motivation: graphql#622 (comment)
Motivation: #622 (comment)
@danielrearden Based on the previous comment I cleaned up Flow typings in #624
Sounds good 👍 |
Motivation: graphql#622 (comment)
This PR only changes the existing TS types.