feat(fetch): split responses in separate types based on status #1885
Conversation
|
Thank you for this improvement. I'm checking other PRs, so I'll check this one in turn. |
packages/fetch/src/index.ts
Outdated
| .map( | ||
| (r) => `export type ${responseTypeName}${pascal(r.key)} = { | ||
| ${isContentTypeNdJson(r.contentType) ? 'stream: Response' : `data: ${r.value || 'unknown'}`} | ||
| status: ${r.key === 'default' ? `Exclude<HTTPStatusCodes, ${nonDefaultStatuses.join(' | ')}>` : r.key} |
There was a problem hiding this comment.
I just realized this probably fails if the spec contains only a default status and no other statuses. Will need to change this, so dont merge yet
|
I converted it to a Draft for now @AllieJonsson so no one merges it. |
There was a problem hiding this comment.
@AllieJonsson
It's not important to me, but it makes the code redundant, so I made a suggestion.
packages/core/src/writers/types.ts
Outdated
| @@ -25,3 +25,12 @@ type NonReadonly<T> = [T] extends [UnionToIntersection<T>] ? { | |||
| : T[P]; | |||
| } : DistributeReadOnlyOverUnions<T>; | |||
| `; | |||
|
|
|||
| export const getHTTPStatusCodes = () => ` | |||
There was a problem hiding this comment.
I'm starting to notice that having this boiler template for each file causes noise.
I think this is a definition for the default, but how about making the default status definition a number instead of a strict enum?
There was a problem hiding this comment.
Unfortunately, that would not work. Image this scenario:
type response200 = {
data: myData;
status: 200;
}
type responseDefault = {
data: myError;
status: number;
}
type responseComposite = response200 | responseDefault;
type response = responseComposite & {
headers: Headers;
}Now checking if status is 200 is not enough:
if (resp.status === 200) {
// resp.data is still typed to myData | myError
}And you cannot exclude certain numbers from number:
type Ex = Exclude<number, 200> is resolved to type Ex = number
There was a problem hiding this comment.
I see, you are right. Please wait a moment as I confirm this.
There was a problem hiding this comment.
@AllieJonsson
I just finished checking everything. Thank you for the great changes 🙌
could you regenerate sample apps?
AllieJonsson
force-pushed
the
feat/fetch-split-status-response
branch
from
929bc8b to
ad498ae
Compare
packages/swr/src/index.ts
Outdated
| ? `type SecondParameter<T extends (...args: any) => any> = Parameters<T>[1];\n\n` | ||
| : '' | ||
| }`; | ||
| } | ||
| ${params.output.httpClient === OutputHttpClient.FETCH ? generateFetchHeader(params) : ''} |
There was a problem hiding this comment.
The functions when processing is divided by httpClient are consolidated into client.ts, so please move them 👍
|
CI is failing 👀 |
|
Hi @AllieJonsson Could you check this? |
|
Yes, sorry, I've had the flu 🤒 I'll fix this later today |
AllieJonsson
force-pushed
the
feat/fetch-split-status-response
branch
from
4bc00a9 to
f98a3f5
Compare
|
@soartec-lab Issue fixed, but there seems to be some zod errors in the samples? |
|
Oh, please take care of yourself. |
|
Hi @AllieJonsson, The issue has been fixed and merged. Could you please update the sample apps again? |
AllieJonsson
force-pushed
the
feat/fetch-split-status-response
branch
from
f98a3f5 to
6da9e66
Compare
AllieJonsson
force-pushed
the
feat/fetch-split-status-response
branch
from
6da9e66 to
32c1782
Compare
Thanks! It looks good now |
soartec-lab
added
enhancement
|
Thank you so much for this fix! 🙏 Do you have any idea when it is likely to be released? |
|
hopefully today @hypnoboutique |
Status
READY
Description
Fix #1881
Splits response types into multiple types, to be able to check against status and get correctly typed data.
Test with specifications with multiple responses, as well as a default response
Todos