Conversation
| } | ||
|
|
||
| async _resolveVariantReferences(variant: Variant) { | ||
| async _resolveVariantReferences(variant: Variant): Promise<Variant> { |
There was a problem hiding this comment.
Flow requires exported functions/interfaces to have an explicit return type declaration
Cannot build a typed interface for this module. You should annotate the exports of this module with types. Missing type annotation at function return:Flow(signature-verification-failure)
There was a problem hiding this comment.
Makes somehow sense to be explicit about the return type.
| /* Client */ | ||
| export type CustomClientResult = ClientResult & { | ||
| id?: string, | ||
| id: string, |
There was a problem hiding this comment.
This should have been required, as far as I can see in the code
| // Set flowtype annotations | ||
| config: AuthOptions | ||
| fetcher: ConfigFetch | ||
| fetcher: typeof fetch |
There was a problem hiding this comment.
We should be able to use the built-in type declarations for the fetch object.
Because of that, we can remove all our custom type declarations.
| expect.objectContaining({ | ||
| headers: { | ||
| 'Content-Type': ['application/json'], | ||
| 'Content-Type': 'application/json', |
There was a problem hiding this comment.
I couldn't recall why we were using an array of values for some headers...I think it makes sense to keep it simple and declare it as a single value
There was a problem hiding this comment.
Your argument makes sense to me too 👍
emmenko
force-pushed
the
nm-upgrade-flow-yarn
branch
from
379e1e2 to
5b7e75a
Compare
emmenko
changed the title
Codecov Report
@@ Coverage Diff @@
## master #1603 +/- ##
==========================================
- Coverage 98.67% 98.62% -0.06%
==========================================
Files 128 128
Lines 3326 3345 +19
Branches 766 773 +7
==========================================
+ Hits 3282 3299 +17
- Misses 40 42 +2
Partials 4 4
Continue to review full report at Codecov.
|
| } | ||
|
|
||
| async _resolveVariantReferences(variant: Variant) { | ||
| async _resolveVariantReferences(variant: Variant): Promise<Variant> { |
There was a problem hiding this comment.
Makes somehow sense to be explicit about the return type.
| expect.objectContaining({ | ||
| headers: { | ||
| 'Content-Type': ['application/json'], | ||
| 'Content-Type': 'application/json', |
There was a problem hiding this comment.
Your argument makes sense to me too 👍
daern91
left a comment
daern91
left a comment
There was a problem hiding this comment.
LGTM 👍 Thx a lot for putting in the effort here
| suppress_type=$FlowIssue | ||
| suppress_type=$FlowFixMe | ||
|
|
||
| # https://medium.com/flow-type/types-first-a-scalable-new-architecture-for-flow-3d8c7ba1d4eb |
There was a problem hiding this comment.
Thanks a lot for adding the links/context
packages/sdk-auth/src/auth.js
Outdated
| /* eslint-disable */ | ||
| fetchFunction = fetch | ||
| /* eslint-enable */ |
There was a problem hiding this comment.
do we disable here because of the reassignment? Doesn't flow allow casting, maybe?
There was a problem hiding this comment.
Ah sorry, this is not necessary anymore. 5dc6357
| let retryCount = 0 | ||
| // wrap in a fn so we can retry if error occur | ||
| function executeFetch() { | ||
| // $FlowFixMe |
There was a problem hiding this comment.
super nice to see this one removed
emmenko
force-pushed
the
nm-upgrade-flow-yarn
branch
from
5dc6357 to
1b965b2
Compare
3 participants
Some maintenance work:
0.131.0. This comes with some overdue required changes, like https://medium.com/flow-type/types-first-a-scalable-new-architecture-for-flow-3d8c7ba1d4ebupgrade yarn bin todone in chore: use latest node versions #16041.22.4