From 08cd22c016f59ca77a01b18883ed5ec7e4cbbfbf Mon Sep 17 00:00:00 2001 From: Igor Savin Date: Mon, 8 Jul 2024 12:57:43 +0300 Subject: [PATCH 01/15] Improve 204 support --- package.json | 6 +-- src/client.ts | 107 +++++++++++++++++++------------------- src/types.ts | 2 + src/utils/bodyUtils.ts | 64 +++++++++++++++++++++++ src/{ => utils}/either.ts | 0 5 files changed, 122 insertions(+), 57 deletions(-) create mode 100644 src/utils/bodyUtils.ts rename src/{ => utils}/either.ts (100%) diff --git a/package.json b/package.json index 8cf41b3..43f99b0 100644 --- a/package.json +++ b/package.json @@ -56,7 +56,7 @@ "@types/node": "^20.11.5", "@typescript-eslint/eslint-plugin": "^7.0.1", "@typescript-eslint/parser": "^7.0.1", - "@vitest/coverage-v8": "^1.2.2", + "@vitest/coverage-v8": "^1.6.0", "jest-fail-on-console": "^3.1.2", "eslint": "^8.56.0", "eslint-plugin-import": "^2.29.1", @@ -66,8 +66,8 @@ "prettier": "^3.2.5", "rimraf": "^5.0.5", "tsup": "8.1.0", - "typescript": "~5.5.2", - "vitest": "^1.2.2" + "typescript": "~5.5.3", + "vitest": "^1.6.0" }, "keywords": [ "frontend", diff --git a/src/client.ts b/src/client.ts index 16e0eb2..06840f8 100644 --- a/src/client.ts +++ b/src/client.ts @@ -1,7 +1,6 @@ import { stringify } from 'fast-querystring' import type { z } from 'zod' -import { type Either, failure, success, isFailure } from './either.js' import type { CommonRequestParams, FreeQueryParams, @@ -9,6 +8,8 @@ import type { ResourceChangeParams, WretchInstance, } from './types.js' +import { tryToResolveJsonBody } from './utils/bodyUtils.js' +import { type Either, failure, success, isFailure } from './utils/either.js' function parseRequestBody({ body, @@ -72,34 +73,6 @@ function parseQueryParams({ return success(`?${stringify(queryParams)}`) } -function parseResponseBody({ - response, - responseBodySchema, - path, -}: { - response: ResponseBody - responseBodySchema?: z.ZodSchema - path: string -}): Either { - if (!responseBodySchema) { - return success(response) - } - - const result = responseBodySchema.safeParse(response) - - if (!result.success) { - console.error({ - path, - response, - error: result.error, - }) - - return failure(result.error) - } - - return success(response) -} - async function sendResourceChange< T extends WretchInstance, ResponseBody, @@ -132,21 +105,32 @@ async function sendResourceChange< return wretch[method](body.result, `${params.path}${queryParams.result}`).res( async (response) => { - if (response.headers.get('content-type')?.includes('application/json')) { - const parsedResponse = parseResponseBody({ - response: (await response.json()) as ResponseBody, - responseBodySchema: params.responseBodySchema, - path: params.path, - }) - - if (isFailure(parsedResponse)) { - return Promise.reject(parsedResponse.error) + if (response.status === 204) { + if (params.isEmptyResponseExpected === false) { + return Promise.reject(new Error(`Request to ${params.path} has returned an unexpected empty response.`)) + } + // It is generally OK for resource change operation to return empty response + return response + } + + const bodyParseResult = await tryToResolveJsonBody( + response, + params.path, + params.responseBodySchema, + ) + + if (bodyParseResult.error === 'NOT_JSON') { + if (params.isNonJSONResponseExpected === false) { + return Promise.reject(new Error(`Request to ${params.path} has returned an unexpected non-JSON response.`)) } + return response as unknown as Promise + } - return parsedResponse.result + if (bodyParseResult.error) { + return Promise.reject(bodyParseResult.error) } - return response as unknown as Promise + return bodyParseResult.result }, ) } @@ -175,22 +159,37 @@ export async function sendGet< return Promise.reject(queryParams.error) } - return wretch - .get(`${params.path}${queryParams.result}`) - .json() - .then((response) => { - const parsedResponse = parseResponseBody({ - response: response as ResponseBody, - responseBodySchema: params.responseBodySchema, - path: params.path, - }) - - if (isFailure(parsedResponse)) { - return Promise.reject(parsedResponse.error) + return wretch.get(`${params.path}${queryParams.result}`).res(async (response) => { + const bodyParseResult = await tryToResolveJsonBody( + response, + params.path, + params.responseBodySchema, + ) + + if (bodyParseResult.error === 'NOT_JSON') { + if (params.isNonJSONResponseExpected) { + return response as unknown as Promise + } + return Promise.reject( + `Request to ${params.path} has returned an unexpected non-JSON response.`, + ) + } + + if (bodyParseResult.error === 'EMPTY_RESPONSE') { + if (params.isEmptyResponseExpected) { + return response as unknown as Promise } + return Promise.reject( + `Request to ${params.path} has returned an unexpected empty response.`, + ) + } - return parsedResponse.result - }) + if (bodyParseResult.error) { + return Promise.reject(bodyParseResult.error) + } + + return bodyParseResult.result + }) } /* POST */ diff --git a/src/types.ts b/src/types.ts index 6bd8b6d..7a46423 100644 --- a/src/types.ts +++ b/src/types.ts @@ -6,6 +6,8 @@ type FreeformRecord = Record export type CommonRequestParams = { path: string responseBodySchema?: ZodSchema + isEmptyResponseExpected?: boolean // 204 is considered a success. Default is "false" for GET operations and "true" for everything else + isNonJSONResponseExpected?: boolean // Do not throw an error if not receiving 'application/json' content-type. Default is "false" for GET operations and "true" for everything else } export type BodyRequestParams = { diff --git a/src/utils/bodyUtils.ts b/src/utils/bodyUtils.ts new file mode 100644 index 0000000..74ba74a --- /dev/null +++ b/src/utils/bodyUtils.ts @@ -0,0 +1,64 @@ +import type { WretchResponse } from 'wretch' +import type { ZodError } from 'zod' +import { z } from 'zod' + +import type { Either } from './either.js' +import { failure, success } from './either.js' + +const ANY_PAYLOAD_SCHEMA = z.any() + +export function tryToResolveJsonBody< + RequestBodySchema extends z.ZodSchema = typeof ANY_PAYLOAD_SCHEMA, +>( + response: WretchResponse, + path: string, + schema: RequestBodySchema = ANY_PAYLOAD_SCHEMA as unknown as RequestBodySchema, +): Promise, z.output>> { + if (response.status === 204) { + return Promise.resolve({ + error: "EMPTY_RESPONSE", + }) + } + + if (!response.headers.get('content-type')?.includes('application/json')) { + return Promise.resolve({ + error: 'NOT_JSON', + }) + } + + return response.json().then((responseBody) => { + return parseResponseBody({ + response: responseBody, + responseBodySchema: schema, + path, + }) + }) +} + +function parseResponseBody({ + response, + responseBodySchema, + path, +}: { + response: ResponseBody + responseBodySchema?: z.ZodSchema + path: string +}): Either { + if (!responseBodySchema) { + return success(response) + } + + const result = responseBodySchema.safeParse(response) + + if (!result.success) { + console.error({ + path, + response, + error: result.error, + }) + + return failure(result.error) + } + + return success(response) +} diff --git a/src/either.ts b/src/utils/either.ts similarity index 100% rename from src/either.ts rename to src/utils/either.ts From 6f205102f577ecfb0b7c8f963a7b48cd6d42ba1d Mon Sep 17 00:00:00 2001 From: CarlosGamero Date: Mon, 8 Jul 2024 12:54:36 +0200 Subject: [PATCH 02/15] Adding isEmptyResponseExpected as false tests --- src/client.test.ts | 51 ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 49 insertions(+), 2 deletions(-) diff --git a/src/client.test.ts b/src/client.test.ts index 2c8bb92..da9b334 100644 --- a/src/client.test.ts +++ b/src/client.test.ts @@ -50,13 +50,23 @@ describe('frontend-http-client', () => { const responseBody = await sendPost(client, { path: '/', }) - expect(responseBody).containSubset({ status: 204, statusText: 'No Content', }) }) + it('returns unexpected no content response', async () => { + const client = wretch(mockServer.url) + + await mockServer.forPost('/').thenReply(204) + + await expect(sendPost(client, { + path: '/', + isEmptyResponseExpected: false + })).rejects.toThrowErrorMatchingInlineSnapshot(`[Error: Request to / has returned an unexpected empty response.]`) + }) + it('throws an error if response does not pass validation', async () => { const client = wretch(mockServer.url) @@ -299,16 +309,27 @@ describe('frontend-http-client', () => { await mockServer.forPut('/').thenReply(204) + const responseBody = await sendPut(client, { path: '/', }) - expect(responseBody).containSubset({ status: 204, statusText: 'No Content', }) }) + it('returns unexpected no content response', async () => { + const client = wretch(mockServer.url) + + await mockServer.forPut('/').thenReply(204) + + await expect(sendPut(client, { + path: '/', + isEmptyResponseExpected: false + })).rejects.toThrowErrorMatchingInlineSnapshot(`[Error: Request to / has returned an unexpected empty response.]`) + }) + it('throws an error if response does not pass validation', async () => { const client = wretch(mockServer.url) @@ -484,6 +505,32 @@ describe('frontend-http-client', () => { }) }) + it('returns no content response', async () => { + const client = wretch(mockServer.url) + + await mockServer.forPatch('/').thenReply(204) + + + const responseBody = await sendPatch(client, { + path: '/', + }) + expect(responseBody).containSubset({ + status: 204, + statusText: 'No Content', + }) + }) + + it('returns unexpected no content response', async () => { + const client = wretch(mockServer.url) + + await mockServer.forPatch('/').thenReply(204) + + await expect(sendPatch(client, { + path: '/', + isEmptyResponseExpected: false + })).rejects.toThrowErrorMatchingInlineSnapshot(`[Error: Request to / has returned an unexpected empty response.]`) + }) + it('throws an error if response does not pass validation', async () => { const client = wretch(mockServer.url) From 8dee473e75d0731cd328dfd0a971725aa9242625 Mon Sep 17 00:00:00 2001 From: CarlosGamero Date: Mon, 8 Jul 2024 13:07:01 +0200 Subject: [PATCH 03/15] 204 sendGet tests --- src/client.test.ts | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/src/client.test.ts b/src/client.test.ts index da9b334..1bd0c03 100644 --- a/src/client.test.ts +++ b/src/client.test.ts @@ -1,6 +1,7 @@ /* eslint-disable max-lines */ import failOnConsole from 'jest-fail-on-console' import { getLocal } from 'mockttp' +import {expect} from "vitest"; import wretch from 'wretch' import { z } from 'zod' @@ -706,6 +707,31 @@ describe('frontend-http-client', () => { }) }) + it('returns no content response', async () => { + const client = wretch(mockServer.url) + + await mockServer.forGet('/').thenReply(204) + + await expect(sendGet(client, { + path: '/', + })).rejects.toThrowErrorMatchingInlineSnapshot(`"Request to / has returned an unexpected empty response."`) + }) + + it('returns expected no content response', async () => { + const client = wretch(mockServer.url) + + await mockServer.forGet('/').thenReply(204) + + const response = await sendGet(client, { + path: '/', + isEmptyResponseExpected: true + }) + expect(response).containSubset({ + status: 204, + statusText: 'No Content', + }) + }) + it('throws an error if response does not pass validation', async () => { const client = wretch(mockServer.url) From d01ff320e7547624fe7ba9bc0d0d22baee06436b Mon Sep 17 00:00:00 2001 From: CarlosGamero Date: Mon, 8 Jul 2024 13:20:37 +0200 Subject: [PATCH 04/15] not json response tests --- src/client.test.ts | 116 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 115 insertions(+), 1 deletion(-) diff --git a/src/client.test.ts b/src/client.test.ts index 1bd0c03..c3cf1b6 100644 --- a/src/client.test.ts +++ b/src/client.test.ts @@ -68,6 +68,31 @@ describe('frontend-http-client', () => { })).rejects.toThrowErrorMatchingInlineSnapshot(`[Error: Request to / has returned an unexpected empty response.]`) }) + it('returns not json response', async () => { + const client = wretch(mockServer.url) + + await mockServer.forPost('/').thenReply(200) + + const responseBody = await sendPost(client, { + path: '/', + }) + expect(responseBody).containSubset({ + status: 200, + statusText: 'OK', + }) + }) + + it('returns unexpected not json response', async () => { + const client = wretch(mockServer.url) + + await mockServer.forPost('/').thenReply(200) + + await expect(sendPost(client, { + path: '/', + isNonJSONResponseExpected: false + })).rejects.toThrowErrorMatchingInlineSnapshot(`[Error: Request to / has returned an unexpected non-JSON response.]`) + }) + it('throws an error if response does not pass validation', async () => { const client = wretch(mockServer.url) @@ -331,6 +356,31 @@ describe('frontend-http-client', () => { })).rejects.toThrowErrorMatchingInlineSnapshot(`[Error: Request to / has returned an unexpected empty response.]`) }) + it('returns not json response', async () => { + const client = wretch(mockServer.url) + + await mockServer.forPut('/').thenReply(200) + + const responseBody = await sendPut(client, { + path: '/', + }) + expect(responseBody).containSubset({ + status: 200, + statusText: 'OK', + }) + }) + + it('returns unexpected not json response', async () => { + const client = wretch(mockServer.url) + + await mockServer.forPut('/').thenReply(200) + + await expect(sendPut(client, { + path: '/', + isNonJSONResponseExpected: false + })).rejects.toThrowErrorMatchingInlineSnapshot(`[Error: Request to / has returned an unexpected non-JSON response.]`) + }) + it('throws an error if response does not pass validation', async () => { const client = wretch(mockServer.url) @@ -532,6 +582,31 @@ describe('frontend-http-client', () => { })).rejects.toThrowErrorMatchingInlineSnapshot(`[Error: Request to / has returned an unexpected empty response.]`) }) + it('returns not json response', async () => { + const client = wretch(mockServer.url) + + await mockServer.forPatch('/').thenReply(200) + + const responseBody = await sendPatch(client, { + path: '/', + }) + expect(responseBody).containSubset({ + status: 200, + statusText: 'OK', + }) + }) + + it('returns unexpected not json response', async () => { + const client = wretch(mockServer.url) + + await mockServer.forPatch('/').thenReply(200) + + await expect(sendPatch(client, { + path: '/', + isNonJSONResponseExpected: false + })).rejects.toThrowErrorMatchingInlineSnapshot(`[Error: Request to / has returned an unexpected non-JSON response.]`) + }) + it('throws an error if response does not pass validation', async () => { const client = wretch(mockServer.url) @@ -732,6 +807,31 @@ describe('frontend-http-client', () => { }) }) + it('returns not json response', async () => { + const client = wretch(mockServer.url) + + await mockServer.forGet('/').thenReply(200) + + await expect(sendGet(client, { + path: '/', + })).rejects.toThrowErrorMatchingInlineSnapshot(`"Request to / has returned an unexpected non-JSON response."`) + }) + + it('returns expected not json response', async () => { + const client = wretch(mockServer.url) + + await mockServer.forGet('/').thenReply(200) + + const responseBody = await sendGet(client, { + path: '/', + isNonJSONResponseExpected: true + }) + expect(responseBody).containSubset({ + status: 200, + statusText: 'OK', + }) + }) + it('throws an error if response does not pass validation', async () => { const client = wretch(mockServer.url) @@ -876,7 +976,7 @@ describe('frontend-http-client', () => { }) describe('sendDelete', () => { - it('returns a status if proceeded', async () => { + it('returns no content response', async () => { const client = wretch(mockServer.url) await mockServer.forDelete('/').thenReply(204) @@ -887,5 +987,19 @@ describe('frontend-http-client', () => { expect(response).toMatchObject({ status: 204 }) }) + + it('returns not json response', async () => { + const client = wretch(mockServer.url) + + await mockServer.forDelete('/').thenReply(200) + + const responseBody = await sendDelete(client, { + path: '/', + }) + expect(responseBody).containSubset({ + status: 200, + statusText: 'OK', + }) + }) }) }) From 906af1c20c6dbbe4504be54e27b36a21ec7ce48f Mon Sep 17 00:00:00 2001 From: CarlosGamero Date: Mon, 8 Jul 2024 13:23:08 +0200 Subject: [PATCH 05/15] Lint fix --- src/utils/bodyUtils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utils/bodyUtils.ts b/src/utils/bodyUtils.ts index 74ba74a..09e407e 100644 --- a/src/utils/bodyUtils.ts +++ b/src/utils/bodyUtils.ts @@ -26,7 +26,7 @@ export function tryToResolveJsonBody< }) } - return response.json().then((responseBody) => { + return response.json().then((responseBody: object) => { return parseResponseBody({ response: responseBody, responseBodySchema: schema, From 59e5e6a4aec8d699e7c9d6d2f1aeb7e63ad88123 Mon Sep 17 00:00:00 2001 From: CarlosGamero Date: Mon, 8 Jul 2024 13:31:15 +0200 Subject: [PATCH 06/15] Doc --- README.md | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/README.md b/README.md index fe8402a..2350530 100644 --- a/README.md +++ b/README.md @@ -35,6 +35,28 @@ const responseBody = await sendPost(client, { }) ``` +### No content response handling (HTTP 204) +SDK methods has a parameter (`isEmptyResponseExpected`) to specify if 204 response should be treated as an error or not. By default it is treated as +valid except on `sendGet` method where it is treated as an error. Usage example: + +```ts +const response = await sendGet(client, { + path: '/', + isEmptyResponseExpected: true, +}) +``` + +### Non json response handling +SDK methods has a parameter (`isNonJSONResponseExpected`) to specify if non json responses should be treated as an error +or not. By default it is treated as valid except on `sendGet` method where it is treated as an error. Usage example: + +```ts +const response = await sendGet(client, { + path: '/', + isNonJSONResponseExpected: true, +}) +``` + ## Credits This library is brought to you by a joint effort of Lokalise engineers: From e324a260f00708b0ba3e9ea0efe23e6807fb7dfb Mon Sep 17 00:00:00 2001 From: CarlosGamero Date: Mon, 8 Jul 2024 13:33:31 +0200 Subject: [PATCH 07/15] Lint fix --- README.md | 12 ++--- src/client.test.ts | 100 ++++++++++++++++++++++++++--------------- src/client.ts | 14 +++--- src/utils/bodyUtils.ts | 14 +++--- 4 files changed, 88 insertions(+), 52 deletions(-) diff --git a/README.md b/README.md index 2350530..d61c429 100644 --- a/README.md +++ b/README.md @@ -36,24 +36,26 @@ const responseBody = await sendPost(client, { ``` ### No content response handling (HTTP 204) -SDK methods has a parameter (`isEmptyResponseExpected`) to specify if 204 response should be treated as an error or not. By default it is treated as + +SDK methods has a parameter (`isEmptyResponseExpected`) to specify if 204 response should be treated as an error or not. By default it is treated as valid except on `sendGet` method where it is treated as an error. Usage example: ```ts const response = await sendGet(client, { - path: '/', - isEmptyResponseExpected: true, + path: '/', + isEmptyResponseExpected: true, }) ``` ### Non json response handling + SDK methods has a parameter (`isNonJSONResponseExpected`) to specify if non json responses should be treated as an error or not. By default it is treated as valid except on `sendGet` method where it is treated as an error. Usage example: ```ts const response = await sendGet(client, { - path: '/', - isNonJSONResponseExpected: true, + path: '/', + isNonJSONResponseExpected: true, }) ``` diff --git a/src/client.test.ts b/src/client.test.ts index c3cf1b6..6c31d0b 100644 --- a/src/client.test.ts +++ b/src/client.test.ts @@ -1,7 +1,7 @@ /* eslint-disable max-lines */ import failOnConsole from 'jest-fail-on-console' import { getLocal } from 'mockttp' -import {expect} from "vitest"; +import { expect } from 'vitest' import wretch from 'wretch' import { z } from 'zod' @@ -62,10 +62,14 @@ describe('frontend-http-client', () => { await mockServer.forPost('/').thenReply(204) - await expect(sendPost(client, { - path: '/', - isEmptyResponseExpected: false - })).rejects.toThrowErrorMatchingInlineSnapshot(`[Error: Request to / has returned an unexpected empty response.]`) + await expect( + sendPost(client, { + path: '/', + isEmptyResponseExpected: false, + }), + ).rejects.toThrowErrorMatchingInlineSnapshot( + `[Error: Request to / has returned an unexpected empty response.]`, + ) }) it('returns not json response', async () => { @@ -87,10 +91,14 @@ describe('frontend-http-client', () => { await mockServer.forPost('/').thenReply(200) - await expect(sendPost(client, { - path: '/', - isNonJSONResponseExpected: false - })).rejects.toThrowErrorMatchingInlineSnapshot(`[Error: Request to / has returned an unexpected non-JSON response.]`) + await expect( + sendPost(client, { + path: '/', + isNonJSONResponseExpected: false, + }), + ).rejects.toThrowErrorMatchingInlineSnapshot( + `[Error: Request to / has returned an unexpected non-JSON response.]`, + ) }) it('throws an error if response does not pass validation', async () => { @@ -335,7 +343,6 @@ describe('frontend-http-client', () => { await mockServer.forPut('/').thenReply(204) - const responseBody = await sendPut(client, { path: '/', }) @@ -350,10 +357,14 @@ describe('frontend-http-client', () => { await mockServer.forPut('/').thenReply(204) - await expect(sendPut(client, { - path: '/', - isEmptyResponseExpected: false - })).rejects.toThrowErrorMatchingInlineSnapshot(`[Error: Request to / has returned an unexpected empty response.]`) + await expect( + sendPut(client, { + path: '/', + isEmptyResponseExpected: false, + }), + ).rejects.toThrowErrorMatchingInlineSnapshot( + `[Error: Request to / has returned an unexpected empty response.]`, + ) }) it('returns not json response', async () => { @@ -375,10 +386,14 @@ describe('frontend-http-client', () => { await mockServer.forPut('/').thenReply(200) - await expect(sendPut(client, { - path: '/', - isNonJSONResponseExpected: false - })).rejects.toThrowErrorMatchingInlineSnapshot(`[Error: Request to / has returned an unexpected non-JSON response.]`) + await expect( + sendPut(client, { + path: '/', + isNonJSONResponseExpected: false, + }), + ).rejects.toThrowErrorMatchingInlineSnapshot( + `[Error: Request to / has returned an unexpected non-JSON response.]`, + ) }) it('throws an error if response does not pass validation', async () => { @@ -561,7 +576,6 @@ describe('frontend-http-client', () => { await mockServer.forPatch('/').thenReply(204) - const responseBody = await sendPatch(client, { path: '/', }) @@ -576,10 +590,14 @@ describe('frontend-http-client', () => { await mockServer.forPatch('/').thenReply(204) - await expect(sendPatch(client, { - path: '/', - isEmptyResponseExpected: false - })).rejects.toThrowErrorMatchingInlineSnapshot(`[Error: Request to / has returned an unexpected empty response.]`) + await expect( + sendPatch(client, { + path: '/', + isEmptyResponseExpected: false, + }), + ).rejects.toThrowErrorMatchingInlineSnapshot( + `[Error: Request to / has returned an unexpected empty response.]`, + ) }) it('returns not json response', async () => { @@ -601,10 +619,14 @@ describe('frontend-http-client', () => { await mockServer.forPatch('/').thenReply(200) - await expect(sendPatch(client, { - path: '/', - isNonJSONResponseExpected: false - })).rejects.toThrowErrorMatchingInlineSnapshot(`[Error: Request to / has returned an unexpected non-JSON response.]`) + await expect( + sendPatch(client, { + path: '/', + isNonJSONResponseExpected: false, + }), + ).rejects.toThrowErrorMatchingInlineSnapshot( + `[Error: Request to / has returned an unexpected non-JSON response.]`, + ) }) it('throws an error if response does not pass validation', async () => { @@ -787,9 +809,13 @@ describe('frontend-http-client', () => { await mockServer.forGet('/').thenReply(204) - await expect(sendGet(client, { - path: '/', - })).rejects.toThrowErrorMatchingInlineSnapshot(`"Request to / has returned an unexpected empty response."`) + await expect( + sendGet(client, { + path: '/', + }), + ).rejects.toThrowErrorMatchingInlineSnapshot( + `"Request to / has returned an unexpected empty response."`, + ) }) it('returns expected no content response', async () => { @@ -799,7 +825,7 @@ describe('frontend-http-client', () => { const response = await sendGet(client, { path: '/', - isEmptyResponseExpected: true + isEmptyResponseExpected: true, }) expect(response).containSubset({ status: 204, @@ -812,9 +838,13 @@ describe('frontend-http-client', () => { await mockServer.forGet('/').thenReply(200) - await expect(sendGet(client, { - path: '/', - })).rejects.toThrowErrorMatchingInlineSnapshot(`"Request to / has returned an unexpected non-JSON response."`) + await expect( + sendGet(client, { + path: '/', + }), + ).rejects.toThrowErrorMatchingInlineSnapshot( + `"Request to / has returned an unexpected non-JSON response."`, + ) }) it('returns expected not json response', async () => { @@ -824,7 +854,7 @@ describe('frontend-http-client', () => { const responseBody = await sendGet(client, { path: '/', - isNonJSONResponseExpected: true + isNonJSONResponseExpected: true, }) expect(responseBody).containSubset({ status: 200, diff --git a/src/client.ts b/src/client.ts index 06840f8..144f57c 100644 --- a/src/client.ts +++ b/src/client.ts @@ -107,7 +107,9 @@ async function sendResourceChange< async (response) => { if (response.status === 204) { if (params.isEmptyResponseExpected === false) { - return Promise.reject(new Error(`Request to ${params.path} has returned an unexpected empty response.`)) + return Promise.reject( + new Error(`Request to ${params.path} has returned an unexpected empty response.`), + ) } // It is generally OK for resource change operation to return empty response return response @@ -121,9 +123,11 @@ async function sendResourceChange< if (bodyParseResult.error === 'NOT_JSON') { if (params.isNonJSONResponseExpected === false) { - return Promise.reject(new Error(`Request to ${params.path} has returned an unexpected non-JSON response.`)) + return Promise.reject( + new Error(`Request to ${params.path} has returned an unexpected non-JSON response.`), + ) } - return response as unknown as Promise + return response as unknown as Promise } if (bodyParseResult.error) { @@ -179,9 +183,7 @@ export async function sendGet< if (params.isEmptyResponseExpected) { return response as unknown as Promise } - return Promise.reject( - `Request to ${params.path} has returned an unexpected empty response.`, - ) + return Promise.reject(`Request to ${params.path} has returned an unexpected empty response.`) } if (bodyParseResult.error) { diff --git a/src/utils/bodyUtils.ts b/src/utils/bodyUtils.ts index 09e407e..aaa791d 100644 --- a/src/utils/bodyUtils.ts +++ b/src/utils/bodyUtils.ts @@ -13,14 +13,16 @@ export function tryToResolveJsonBody< response: WretchResponse, path: string, schema: RequestBodySchema = ANY_PAYLOAD_SCHEMA as unknown as RequestBodySchema, -): Promise, z.output>> { +): Promise< + Either<'NOT_JSON' | 'EMPTY_RESPONSE' | ZodError, z.output> +> { if (response.status === 204) { - return Promise.resolve({ - error: "EMPTY_RESPONSE", - }) - } + return Promise.resolve({ + error: 'EMPTY_RESPONSE', + }) + } - if (!response.headers.get('content-type')?.includes('application/json')) { + if (!response.headers.get('content-type')?.includes('application/json')) { return Promise.resolve({ error: 'NOT_JSON', }) From 8105613cda0697f1ffec0f72d9cab4b8d1ca07b1 Mon Sep 17 00:00:00 2001 From: CarlosGamero Date: Mon, 8 Jul 2024 13:43:23 +0200 Subject: [PATCH 08/15] PR comments --- README.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index d61c429..417b3ed 100644 --- a/README.md +++ b/README.md @@ -47,7 +47,9 @@ const response = await sendGet(client, { }) ``` -### Non json response handling +if `204` responses are expected, the library will return the entire response object, if not, it will throw an error. + +### Non-JSON response handling SDK methods has a parameter (`isNonJSONResponseExpected`) to specify if non json responses should be treated as an error or not. By default it is treated as valid except on `sendGet` method where it is treated as an error. Usage example: @@ -58,6 +60,7 @@ const response = await sendGet(client, { isNonJSONResponseExpected: true, }) ``` +if non-JSON responses are expected, the library will return the entire response object, if not, it will throw an error. ## Credits From 578af6de1a9274c31237d9a75eb5d93009120bdc Mon Sep 17 00:00:00 2001 From: CarlosGamero Date: Mon, 8 Jul 2024 15:11:02 +0200 Subject: [PATCH 09/15] 204 returns null + standard check for all methods + explicit return type --- README.md | 4 ++-- src/client.test.ts | 25 +++++++------------------ src/client.ts | 35 ++++++++++++++++++----------------- 3 files changed, 27 insertions(+), 37 deletions(-) diff --git a/README.md b/README.md index 417b3ed..9c02e6d 100644 --- a/README.md +++ b/README.md @@ -47,7 +47,7 @@ const response = await sendGet(client, { }) ``` -if `204` responses are expected, the library will return the entire response object, if not, it will throw an error. +if `204` responses are expected, the library will return null, if not, it will throw an error. ### Non-JSON response handling @@ -60,7 +60,7 @@ const response = await sendGet(client, { isNonJSONResponseExpected: true, }) ``` -if non-JSON responses are expected, the library will return the entire response object, if not, it will throw an error. +if non-JSON responses are expected, the library will return null, if not, it will throw an error. ## Credits diff --git a/src/client.test.ts b/src/client.test.ts index 6c31d0b..6b32142 100644 --- a/src/client.test.ts +++ b/src/client.test.ts @@ -51,10 +51,7 @@ describe('frontend-http-client', () => { const responseBody = await sendPost(client, { path: '/', }) - expect(responseBody).containSubset({ - status: 204, - statusText: 'No Content', - }) + expect(responseBody).toBe(null) }) it('returns unexpected no content response', async () => { @@ -346,10 +343,7 @@ describe('frontend-http-client', () => { const responseBody = await sendPut(client, { path: '/', }) - expect(responseBody).containSubset({ - status: 204, - statusText: 'No Content', - }) + expect(responseBody).toBe(null) }) it('returns unexpected no content response', async () => { @@ -579,10 +573,7 @@ describe('frontend-http-client', () => { const responseBody = await sendPatch(client, { path: '/', }) - expect(responseBody).containSubset({ - status: 204, - statusText: 'No Content', - }) + expect(responseBody).toBe(null) }) it('returns unexpected no content response', async () => { @@ -827,10 +818,7 @@ describe('frontend-http-client', () => { path: '/', isEmptyResponseExpected: true, }) - expect(response).containSubset({ - status: 204, - statusText: 'No Content', - }) + expect(response).toBe(null) }) it('returns not json response', async () => { @@ -856,6 +844,7 @@ describe('frontend-http-client', () => { path: '/', isNonJSONResponseExpected: true, }) + expect(responseBody).containSubset({ status: 200, statusText: 'OK', @@ -948,7 +937,7 @@ describe('frontend-http-client', () => { responseBodySchema: responseSchema, }) - expect(response.data.code).toBe(99) + expect(response?.data.code).toBe(99) }) it('should work without specifying an schema', async () => { @@ -971,7 +960,7 @@ describe('frontend-http-client', () => { responseBodySchema: responseSchema, }) - expect(response.data.code).toBe(99) + expect(response?.data.code).toBe(99) }) it('should check types against schema input type', async () => { diff --git a/src/client.ts b/src/client.ts index 144f57c..f08b7e9 100644 --- a/src/client.ts +++ b/src/client.ts @@ -1,4 +1,5 @@ import { stringify } from 'fast-querystring' +import type {WretchResponse} from "wretch"; import type { z } from 'zod' import type { @@ -82,7 +83,7 @@ async function sendResourceChange< wretch: T, method: 'post' | 'put' | 'patch', params: ResourceChangeParams, -) { +): Promise { const body = parseRequestBody({ body: params.body, requestBodySchema: params.requestBodySchema, @@ -105,16 +106,6 @@ async function sendResourceChange< return wretch[method](body.result, `${params.path}${queryParams.result}`).res( async (response) => { - if (response.status === 204) { - if (params.isEmptyResponseExpected === false) { - return Promise.reject( - new Error(`Request to ${params.path} has returned an unexpected empty response.`), - ) - } - // It is generally OK for resource change operation to return empty response - return response - } - const bodyParseResult = await tryToResolveJsonBody( response, params.path, @@ -130,6 +121,16 @@ async function sendResourceChange< return response as unknown as Promise } + if (bodyParseResult.error === 'EMPTY_RESPONSE') { + if (params.isEmptyResponseExpected === false) { + return Promise.reject( + new Error(`Request to ${params.path} has returned an unexpected empty response.`, + )) + } + + return null + } + if (bodyParseResult.error) { return Promise.reject(bodyParseResult.error) } @@ -152,7 +153,7 @@ export async function sendGet< params: RequestQuerySchema extends z.Schema ? QueryParams : FreeQueryParams, -): Promise { +): Promise { const queryParams = parseQueryParams({ queryParams: params.queryParams, queryParamsSchema: params.queryParamsSchema, @@ -181,7 +182,7 @@ export async function sendGet< if (bodyParseResult.error === 'EMPTY_RESPONSE') { if (params.isEmptyResponseExpected) { - return response as unknown as Promise + return null } return Promise.reject(`Request to ${params.path} has returned an unexpected empty response.`) } @@ -201,7 +202,7 @@ export function sendPost< ResponseBody, RequestBodySchema extends z.Schema | undefined = undefined, RequestQuerySchema extends z.Schema | undefined = undefined, ->(wretch: T, params: ResourceChangeParams) { +>(wretch: T, params: ResourceChangeParams): Promise { return sendResourceChange(wretch, 'post', params) } @@ -212,7 +213,7 @@ export function sendPut< ResponseBody, RequestBodySchema extends z.Schema | undefined = undefined, RequestQuerySchema extends z.Schema | undefined = undefined, ->(wretch: T, params: ResourceChangeParams) { +>(wretch: T, params: ResourceChangeParams): Promise { return sendResourceChange(wretch, 'put', params) } @@ -223,7 +224,7 @@ export function sendPatch< ResponseBody, RequestBodySchema extends z.Schema | undefined = undefined, RequestQuerySchema extends z.Schema | undefined = undefined, ->(wretch: T, params: ResourceChangeParams) { +>(wretch: T, params: ResourceChangeParams): Promise { return sendResourceChange(wretch, 'patch', params) } @@ -232,6 +233,6 @@ export function sendPatch< export function sendDelete( wretch: T, params: Pick, 'path'>, -) { +): Promise { return wretch.delete(params.path).res() } From d5a61c9510ec60f2ddf1efeb9f1f11da443ad062 Mon Sep 17 00:00:00 2001 From: CarlosGamero Date: Mon, 8 Jul 2024 15:17:21 +0200 Subject: [PATCH 10/15] Using WretchError --- README.md | 1 + src/client.test.ts | 4 ++-- src/client.ts | 36 ++++++++++++++++++++++++++---------- 3 files changed, 29 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index 9c02e6d..3e735b0 100644 --- a/README.md +++ b/README.md @@ -60,6 +60,7 @@ const response = await sendGet(client, { isNonJSONResponseExpected: true, }) ``` + if non-JSON responses are expected, the library will return null, if not, it will throw an error. ## Credits diff --git a/src/client.test.ts b/src/client.test.ts index 6b32142..a6f8d00 100644 --- a/src/client.test.ts +++ b/src/client.test.ts @@ -805,7 +805,7 @@ describe('frontend-http-client', () => { path: '/', }), ).rejects.toThrowErrorMatchingInlineSnapshot( - `"Request to / has returned an unexpected empty response."`, + `[Error: Request to / has returned an unexpected empty response.]`, ) }) @@ -831,7 +831,7 @@ describe('frontend-http-client', () => { path: '/', }), ).rejects.toThrowErrorMatchingInlineSnapshot( - `"Request to / has returned an unexpected non-JSON response."`, + `[Error: Request to / has returned an unexpected non-JSON response.]`, ) }) diff --git a/src/client.ts b/src/client.ts index f08b7e9..44f9ec7 100644 --- a/src/client.ts +++ b/src/client.ts @@ -1,5 +1,6 @@ import { stringify } from 'fast-querystring' -import type {WretchResponse} from "wretch"; +import type { WretchResponse } from 'wretch' +import factory from 'wretch' import type { z } from 'zod' import type { @@ -12,6 +13,8 @@ import type { import { tryToResolveJsonBody } from './utils/bodyUtils.js' import { type Either, failure, success, isFailure } from './utils/either.js' +import WretchError = factory.WretchError + function parseRequestBody({ body, requestBodySchema, @@ -83,7 +86,7 @@ async function sendResourceChange< wretch: T, method: 'post' | 'put' | 'patch', params: ResourceChangeParams, -): Promise { +): Promise { const body = parseRequestBody({ body: params.body, requestBodySchema: params.requestBodySchema, @@ -115,7 +118,9 @@ async function sendResourceChange< if (bodyParseResult.error === 'NOT_JSON') { if (params.isNonJSONResponseExpected === false) { return Promise.reject( - new Error(`Request to ${params.path} has returned an unexpected non-JSON response.`), + new WretchError( + `Request to ${params.path} has returned an unexpected non-JSON response.`, + ), ) } return response as unknown as Promise @@ -124,8 +129,8 @@ async function sendResourceChange< if (bodyParseResult.error === 'EMPTY_RESPONSE') { if (params.isEmptyResponseExpected === false) { return Promise.reject( - new Error(`Request to ${params.path} has returned an unexpected empty response.`, - )) + new WretchError(`Request to ${params.path} has returned an unexpected empty response.`), + ) } return null @@ -176,7 +181,7 @@ export async function sendGet< return response as unknown as Promise } return Promise.reject( - `Request to ${params.path} has returned an unexpected non-JSON response.`, + new WretchError(`Request to ${params.path} has returned an unexpected non-JSON response.`), ) } @@ -184,7 +189,9 @@ export async function sendGet< if (params.isEmptyResponseExpected) { return null } - return Promise.reject(`Request to ${params.path} has returned an unexpected empty response.`) + return Promise.reject( + new WretchError(`Request to ${params.path} has returned an unexpected empty response.`), + ) } if (bodyParseResult.error) { @@ -202,7 +209,10 @@ export function sendPost< ResponseBody, RequestBodySchema extends z.Schema | undefined = undefined, RequestQuerySchema extends z.Schema | undefined = undefined, ->(wretch: T, params: ResourceChangeParams): Promise { +>( + wretch: T, + params: ResourceChangeParams, +): Promise { return sendResourceChange(wretch, 'post', params) } @@ -213,7 +223,10 @@ export function sendPut< ResponseBody, RequestBodySchema extends z.Schema | undefined = undefined, RequestQuerySchema extends z.Schema | undefined = undefined, ->(wretch: T, params: ResourceChangeParams): Promise { +>( + wretch: T, + params: ResourceChangeParams, +): Promise { return sendResourceChange(wretch, 'put', params) } @@ -224,7 +237,10 @@ export function sendPatch< ResponseBody, RequestBodySchema extends z.Schema | undefined = undefined, RequestQuerySchema extends z.Schema | undefined = undefined, ->(wretch: T, params: ResourceChangeParams): Promise { +>( + wretch: T, + params: ResourceChangeParams, +): Promise { return sendResourceChange(wretch, 'patch', params) } From 87875f76a6ff5c0b730cb293ad0734667c783951 Mon Sep 17 00:00:00 2001 From: CarlosGamero Date: Mon, 8 Jul 2024 15:25:46 +0200 Subject: [PATCH 11/15] responseBodySchema mandatory --- src/client.test.ts | 28 ++++++++++++++++------------ src/types.ts | 2 +- 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/src/client.test.ts b/src/client.test.ts index a6f8d00..3a54241 100644 --- a/src/client.test.ts +++ b/src/client.test.ts @@ -50,6 +50,7 @@ describe('frontend-http-client', () => { const responseBody = await sendPost(client, { path: '/', + responseBodySchema: z.any(), }) expect(responseBody).toBe(null) }) @@ -63,6 +64,7 @@ describe('frontend-http-client', () => { sendPost(client, { path: '/', isEmptyResponseExpected: false, + responseBodySchema: z.any(), }), ).rejects.toThrowErrorMatchingInlineSnapshot( `[Error: Request to / has returned an unexpected empty response.]`, @@ -76,6 +78,7 @@ describe('frontend-http-client', () => { const responseBody = await sendPost(client, { path: '/', + responseBodySchema: z.any(), }) expect(responseBody).containSubset({ status: 200, @@ -92,6 +95,7 @@ describe('frontend-http-client', () => { sendPost(client, { path: '/', isNonJSONResponseExpected: false, + responseBodySchema: z.any(), }), ).rejects.toThrowErrorMatchingInlineSnapshot( `[Error: Request to / has returned an unexpected non-JSON response.]`, @@ -272,18 +276,6 @@ describe('frontend-http-client', () => { expect(response).toEqual({ success: true }) }) - it('allows posting request without responseBodySchema', async () => { - const client = wretch(mockServer.url) - - await mockServer.forPost('/').thenJson(200, { success: true }) - - const response = await sendPost(client, { - path: '/', - }) - - expect(response).toEqual({ success: true }) - }) - it('should check types against schema input type', async () => { const client = wretch(mockServer.url) await mockServer.forPost('/').thenJson(200, { success: true }) @@ -342,6 +334,7 @@ describe('frontend-http-client', () => { const responseBody = await sendPut(client, { path: '/', + responseBodySchema: z.any(), }) expect(responseBody).toBe(null) }) @@ -354,6 +347,7 @@ describe('frontend-http-client', () => { await expect( sendPut(client, { path: '/', + responseBodySchema: z.any(), isEmptyResponseExpected: false, }), ).rejects.toThrowErrorMatchingInlineSnapshot( @@ -368,6 +362,7 @@ describe('frontend-http-client', () => { const responseBody = await sendPut(client, { path: '/', + responseBodySchema: z.any(), }) expect(responseBody).containSubset({ status: 200, @@ -383,6 +378,7 @@ describe('frontend-http-client', () => { await expect( sendPut(client, { path: '/', + responseBodySchema: z.any(), isNonJSONResponseExpected: false, }), ).rejects.toThrowErrorMatchingInlineSnapshot( @@ -572,6 +568,7 @@ describe('frontend-http-client', () => { const responseBody = await sendPatch(client, { path: '/', + responseBodySchema: z.any(), }) expect(responseBody).toBe(null) }) @@ -584,6 +581,7 @@ describe('frontend-http-client', () => { await expect( sendPatch(client, { path: '/', + responseBodySchema: z.any(), isEmptyResponseExpected: false, }), ).rejects.toThrowErrorMatchingInlineSnapshot( @@ -598,6 +596,7 @@ describe('frontend-http-client', () => { const responseBody = await sendPatch(client, { path: '/', + responseBodySchema: z.any(), }) expect(responseBody).containSubset({ status: 200, @@ -613,6 +612,7 @@ describe('frontend-http-client', () => { await expect( sendPatch(client, { path: '/', + responseBodySchema: z.any(), isNonJSONResponseExpected: false, }), ).rejects.toThrowErrorMatchingInlineSnapshot( @@ -803,6 +803,7 @@ describe('frontend-http-client', () => { await expect( sendGet(client, { path: '/', + responseBodySchema: z.any(), }), ).rejects.toThrowErrorMatchingInlineSnapshot( `[Error: Request to / has returned an unexpected empty response.]`, @@ -816,6 +817,7 @@ describe('frontend-http-client', () => { const response = await sendGet(client, { path: '/', + responseBodySchema: z.any(), isEmptyResponseExpected: true, }) expect(response).toBe(null) @@ -829,6 +831,7 @@ describe('frontend-http-client', () => { await expect( sendGet(client, { path: '/', + responseBodySchema: z.any(), }), ).rejects.toThrowErrorMatchingInlineSnapshot( `[Error: Request to / has returned an unexpected non-JSON response.]`, @@ -842,6 +845,7 @@ describe('frontend-http-client', () => { const responseBody = await sendGet(client, { path: '/', + responseBodySchema: z.any(), isNonJSONResponseExpected: true, }) diff --git a/src/types.ts b/src/types.ts index 7a46423..dd7a921 100644 --- a/src/types.ts +++ b/src/types.ts @@ -5,7 +5,7 @@ type FreeformRecord = Record export type CommonRequestParams = { path: string - responseBodySchema?: ZodSchema + responseBodySchema: ZodSchema isEmptyResponseExpected?: boolean // 204 is considered a success. Default is "false" for GET operations and "true" for everything else isNonJSONResponseExpected?: boolean // Do not throw an error if not receiving 'application/json' content-type. Default is "false" for GET operations and "true" for everything else } From 85d9e544058e01b8e2da9feac13aa3479d8c6202 Mon Sep 17 00:00:00 2001 From: CarlosGamero Date: Mon, 8 Jul 2024 15:40:25 +0200 Subject: [PATCH 12/15] Filling wretch error fields --- src/client.ts | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/src/client.ts b/src/client.ts index 44f9ec7..dddf498 100644 --- a/src/client.ts +++ b/src/client.ts @@ -1,6 +1,6 @@ import { stringify } from 'fast-querystring' import type { WretchResponse } from 'wretch' -import factory from 'wretch' +import {WretchError} from "wretch/resolver"; import type { z } from 'zod' import type { @@ -13,8 +13,6 @@ import type { import { tryToResolveJsonBody } from './utils/bodyUtils.js' import { type Either, failure, success, isFailure } from './utils/either.js' -import WretchError = factory.WretchError - function parseRequestBody({ body, requestBodySchema, @@ -118,8 +116,8 @@ async function sendResourceChange< if (bodyParseResult.error === 'NOT_JSON') { if (params.isNonJSONResponseExpected === false) { return Promise.reject( - new WretchError( - `Request to ${params.path} has returned an unexpected non-JSON response.`, + buildWretchError( + `Request to ${params.path} has returned an unexpected non-JSON response.`, response ), ) } @@ -129,7 +127,7 @@ async function sendResourceChange< if (bodyParseResult.error === 'EMPTY_RESPONSE') { if (params.isEmptyResponseExpected === false) { return Promise.reject( - new WretchError(`Request to ${params.path} has returned an unexpected empty response.`), + buildWretchError(`Request to ${params.path} has returned an unexpected empty response.`, response), ) } @@ -145,6 +143,15 @@ async function sendResourceChange< ) } +function buildWretchError(message: string, response: WretchResponse): WretchError { + const error = new WretchError(message) + error.response = response + error.status = response.status + error.url = response.url + + return error +} + /* METHODS */ /* GET */ @@ -181,7 +188,7 @@ export async function sendGet< return response as unknown as Promise } return Promise.reject( - new WretchError(`Request to ${params.path} has returned an unexpected non-JSON response.`), + buildWretchError(`Request to ${params.path} has returned an unexpected non-JSON response.`, response), ) } @@ -190,7 +197,7 @@ export async function sendGet< return null } return Promise.reject( - new WretchError(`Request to ${params.path} has returned an unexpected empty response.`), + buildWretchError(`Request to ${params.path} has returned an unexpected empty response.`, response), ) } From 707a0cc0262c6eb5792dea5fb4a3f9da551e7b20 Mon Sep 17 00:00:00 2001 From: CarlosGamero Date: Mon, 8 Jul 2024 15:42:41 +0200 Subject: [PATCH 13/15] lint fix --- src/client.ts | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/src/client.ts b/src/client.ts index dddf498..e9f73d0 100644 --- a/src/client.ts +++ b/src/client.ts @@ -1,6 +1,6 @@ import { stringify } from 'fast-querystring' import type { WretchResponse } from 'wretch' -import {WretchError} from "wretch/resolver"; +import { WretchError } from 'wretch/resolver' import type { z } from 'zod' import type { @@ -117,7 +117,8 @@ async function sendResourceChange< if (params.isNonJSONResponseExpected === false) { return Promise.reject( buildWretchError( - `Request to ${params.path} has returned an unexpected non-JSON response.`, response + `Request to ${params.path} has returned an unexpected non-JSON response.`, + response, ), ) } @@ -127,7 +128,10 @@ async function sendResourceChange< if (bodyParseResult.error === 'EMPTY_RESPONSE') { if (params.isEmptyResponseExpected === false) { return Promise.reject( - buildWretchError(`Request to ${params.path} has returned an unexpected empty response.`, response), + buildWretchError( + `Request to ${params.path} has returned an unexpected empty response.`, + response, + ), ) } @@ -188,7 +192,10 @@ export async function sendGet< return response as unknown as Promise } return Promise.reject( - buildWretchError(`Request to ${params.path} has returned an unexpected non-JSON response.`, response), + buildWretchError( + `Request to ${params.path} has returned an unexpected non-JSON response.`, + response, + ), ) } @@ -197,7 +204,10 @@ export async function sendGet< return null } return Promise.reject( - buildWretchError(`Request to ${params.path} has returned an unexpected empty response.`, response), + buildWretchError( + `Request to ${params.path} has returned an unexpected empty response.`, + response, + ), ) } From d8424cad3e0c4b8eb72a5631ea445a67e5db5766 Mon Sep 17 00:00:00 2001 From: Igor Savin Date: Mon, 8 Jul 2024 18:32:09 +0300 Subject: [PATCH 14/15] Make handling of DELETE more consistent --- package.json | 2 +- src/client.test.ts | 95 ++++++++++++++++++++++++++++++++++++- src/client.ts | 115 +++++++++++++++++++++++++++++++++++++-------- src/types.ts | 64 +++++++++++++++++++------ vitest.config.ts | 12 +++-- 5 files changed, 248 insertions(+), 40 deletions(-) diff --git a/package.json b/package.json index 43f99b0..9463a97 100644 --- a/package.json +++ b/package.json @@ -41,7 +41,7 @@ "clean": "rimraf dist .eslintcache", "lint": "eslint --cache --max-warnings=0 . && prettier --check src \"**/*.{json,md,ts,tsx}\" && tsc --noEmit", "lint:fix": "prettier --write src \"**/*.{json,md,ts,tsx}\" --log-level=warn && eslint . --fix", - "test": "vitest run", + "test": "vitest run --coverage", "prepublishOnly": "npm run clean && npm run build:release" }, "dependencies": { diff --git a/src/client.test.ts b/src/client.test.ts index 3a54241..9eb293a 100644 --- a/src/client.test.ts +++ b/src/client.test.ts @@ -1008,10 +1008,86 @@ describe('frontend-http-client', () => { path: '/', }) - expect(response).toMatchObject({ status: 204 }) + expect(response).toBeNull() }) - it('returns not json response', async () => { + it('validates response if schema is provided', async () => { + const client = wretch(mockServer.url) + + await mockServer.forDelete('/').thenJson(200, { string: 1 }) + + await expect( + sendDelete(client, { + path: '/', + responseBodySchema: z.object({ + string: z.string(), + }), + }), + ).rejects.toThrowErrorMatchingInlineSnapshot(` + [ZodError: [ + { + "code": "invalid_type", + "expected": "string", + "received": "number", + "path": [ + "string" + ], + "message": "Expected string, received number" + } + ]] + `) + }) + + it('returns validated response if schema is provided and response is correct', async () => { + const client = wretch(mockServer.url) + + await mockServer.forDelete('/').thenJson(200, { string: '1' }) + + const response = await sendDelete(client, { + path: '/', + responseBodySchema: z.object({ + string: z.string(), + }), + }) + + expect(response).toMatchInlineSnapshot(` + { + "string": "1", + } + `) + }) + + it('supports query params', async () => { + const client = wretch(mockServer.url) + const testQueryParams = { param1: 'test', param2: 'test' } + + await mockServer.forDelete().withQuery(testQueryParams).thenReply(204) + + const response = await sendDelete(client, { + path: '/', + queryParams: testQueryParams, + }) + + expect(response).toBeNull() + }) + + it('throws if content is expected, but response is empty', async () => { + const client = wretch(mockServer.url) + + await mockServer.forDelete('/').thenReply(204) + + await expect( + sendDelete(client, { + path: '/', + isEmptyResponseExpected: false, + responseBodySchema: z.any(), + }), + ).rejects.toThrowErrorMatchingInlineSnapshot( + `[Error: Request to / has returned an unexpected empty response.]`, + ) + }) + + it('returns non-JSON response', async () => { const client = wretch(mockServer.url) await mockServer.forDelete('/').thenReply(200) @@ -1024,5 +1100,20 @@ describe('frontend-http-client', () => { statusText: 'OK', }) }) + + it('returns unexpected non-JSON response', async () => { + const client = wretch(mockServer.url) + + await mockServer.forDelete('/').thenReply(200) + + await expect( + sendDelete(client, { + path: '/', + isNonJSONResponseExpected: false, + }), + ).rejects.toThrowErrorMatchingInlineSnapshot( + `[Error: Request to / has returned an unexpected non-JSON response.]`, + ) + }) }) }) diff --git a/src/client.ts b/src/client.ts index e9f73d0..7e672c5 100644 --- a/src/client.ts +++ b/src/client.ts @@ -4,9 +4,11 @@ import { WretchError } from 'wretch/resolver' import type { z } from 'zod' import type { - CommonRequestParams, + DeleteParams, + FreeDeleteParams, FreeQueryParams, QueryParams, + RequestResultType, ResourceChangeParams, WretchInstance, } from './types.js' @@ -80,11 +82,17 @@ async function sendResourceChange< ResponseBody, RequestBodySchema extends z.Schema | undefined = undefined, RequestQuerySchema extends z.Schema | undefined = undefined, + IsNonJSONResponseExpected extends boolean = false, >( wretch: T, method: 'post' | 'put' | 'patch', - params: ResourceChangeParams, -): Promise { + params: ResourceChangeParams< + RequestBodySchema, + ResponseBody, + RequestQuerySchema, + IsNonJSONResponseExpected + >, +): Promise> { const body = parseRequestBody({ body: params.body, requestBodySchema: params.requestBodySchema, @@ -122,7 +130,7 @@ async function sendResourceChange< ), ) } - return response as unknown as Promise + return response } if (bodyParseResult.error === 'EMPTY_RESPONSE') { @@ -144,7 +152,7 @@ async function sendResourceChange< return bodyParseResult.result }, - ) + ) as Promise> } function buildWretchError(message: string, response: WretchResponse): WretchError { @@ -164,12 +172,13 @@ export async function sendGet< T extends WretchInstance, ResponseBody, RequestQuerySchema extends z.Schema | undefined = undefined, + IsNonJSONResponseExpected extends boolean = false, >( wretch: T, params: RequestQuerySchema extends z.Schema - ? QueryParams - : FreeQueryParams, -): Promise { + ? QueryParams + : FreeQueryParams, +): Promise> { const queryParams = parseQueryParams({ queryParams: params.queryParams, queryParamsSchema: params.queryParamsSchema, @@ -189,7 +198,7 @@ export async function sendGet< if (bodyParseResult.error === 'NOT_JSON') { if (params.isNonJSONResponseExpected) { - return response as unknown as Promise + return response } return Promise.reject( buildWretchError( @@ -216,7 +225,7 @@ export async function sendGet< } return bodyParseResult.result - }) + }) as RequestResultType } /* POST */ @@ -226,10 +235,16 @@ export function sendPost< ResponseBody, RequestBodySchema extends z.Schema | undefined = undefined, RequestQuerySchema extends z.Schema | undefined = undefined, + IsNonJSONResponseExpected extends boolean = false, >( wretch: T, - params: ResourceChangeParams, -): Promise { + params: ResourceChangeParams< + RequestBodySchema, + ResponseBody, + RequestQuerySchema, + IsNonJSONResponseExpected + >, +): Promise> { return sendResourceChange(wretch, 'post', params) } @@ -240,10 +255,11 @@ export function sendPut< ResponseBody, RequestBodySchema extends z.Schema | undefined = undefined, RequestQuerySchema extends z.Schema | undefined = undefined, + IsNonJSONResponseExpected extends boolean = false, >( wretch: T, params: ResourceChangeParams, -): Promise { +): Promise> { return sendResourceChange(wretch, 'put', params) } @@ -254,18 +270,79 @@ export function sendPatch< ResponseBody, RequestBodySchema extends z.Schema | undefined = undefined, RequestQuerySchema extends z.Schema | undefined = undefined, + IsNonJSONResponseExpected extends boolean = false, >( wretch: T, - params: ResourceChangeParams, -): Promise { + params: ResourceChangeParams< + RequestBodySchema, + ResponseBody, + RequestQuerySchema, + IsNonJSONResponseExpected + >, +): Promise> { return sendResourceChange(wretch, 'patch', params) } /* DELETE */ -export function sendDelete( +export function sendDelete< + T extends WretchInstance, + ResponseBody, + RequestQuerySchema extends z.Schema | undefined = undefined, + IsNonJSONResponseExpected extends boolean = false, +>( wretch: T, - params: Pick, 'path'>, -): Promise { - return wretch.delete(params.path).res() + params: RequestQuerySchema extends z.Schema + ? DeleteParams + : FreeDeleteParams, +): Promise> { + const queryParams = parseQueryParams({ + queryParams: params.queryParams, + queryParamsSchema: params.queryParamsSchema, + path: params.path, + }) + + if (isFailure(queryParams)) { + return Promise.reject(queryParams.error) + } + + return wretch.delete(`${params.path}${queryParams.result}`).res(async (response) => { + const bodyParseResult = await tryToResolveJsonBody( + response, + params.path, + params.responseBodySchema, + ) + + if (bodyParseResult.error === 'NOT_JSON') { + if (params.isNonJSONResponseExpected === false) { + return Promise.reject( + buildWretchError( + `Request to ${params.path} has returned an unexpected non-JSON response.`, + response, + ), + ) + } + + return response + } + + if (bodyParseResult.error === 'EMPTY_RESPONSE') { + if (params.isEmptyResponseExpected === false) { + return Promise.reject( + buildWretchError( + `Request to ${params.path} has returned an unexpected empty response.`, + response, + ), + ) + } + + return null + } + + if (bodyParseResult.error) { + return Promise.reject(bodyParseResult.error) + } + + return bodyParseResult.result + }) as Promise> } diff --git a/src/types.ts b/src/types.ts index dd7a921..e717b25 100644 --- a/src/types.ts +++ b/src/types.ts @@ -1,45 +1,79 @@ -import type { Wretch } from 'wretch' +import type { Wretch, WretchResponse } from 'wretch' import type { ZodSchema, z } from 'zod' type FreeformRecord = Record -export type CommonRequestParams = { +export type CommonRequestParams = { path: string responseBodySchema: ZodSchema isEmptyResponseExpected?: boolean // 204 is considered a success. Default is "false" for GET operations and "true" for everything else - isNonJSONResponseExpected?: boolean // Do not throw an error if not receiving 'application/json' content-type. Default is "false" for GET operations and "true" for everything else + isNonJSONResponseExpected?: IsNonJSONResponseExpected // Do not throw an error if not receiving 'application/json' content-type. Default is "false" for GET operations and "true" for everything else } -export type BodyRequestParams = { +export type BodyRequestParams< + RequestBodySchema extends z.ZodSchema, + ResponseBody, + IsNonJSONResponseExpected extends boolean, +> = { body: z.input | undefined requestBodySchema: RequestBodySchema | undefined -} & CommonRequestParams +} & CommonRequestParams -export type FreeBodyRequestParams = { +export type FreeBodyRequestParams = { body?: FreeformRecord requestBodySchema?: never -} & CommonRequestParams +} & CommonRequestParams -export type QueryParams = { +export type QueryParams< + RequestQuerySchema extends z.ZodSchema, + ResponseBody, + IsNonJSONResponseExpected extends boolean, +> = { queryParams: z.input | undefined queryParamsSchema: RequestQuerySchema | undefined -} & CommonRequestParams +} & CommonRequestParams -export type FreeQueryParams = { +export type FreeQueryParams = { queryParams?: FreeformRecord queryParamsSchema?: never -} & CommonRequestParams +} & CommonRequestParams + +export type DeleteParams< + RequestQuerySchema extends z.ZodSchema, + ResponseBody, + IsNonJSONResponseExpected extends boolean, +> = { + queryParams: z.input | undefined + queryParamsSchema: RequestQuerySchema | undefined +} & Omit, 'responseBodySchema'> & { + responseBodySchema?: ZodSchema + } + +export type FreeDeleteParams = { + queryParams?: FreeformRecord + queryParamsSchema?: never +} & Omit, 'responseBodySchema'> & { + responseBodySchema?: ZodSchema + } + +export type RequestResultType< + ResponseBody, + isNonJSONResponseExpected extends boolean | undefined, +> = isNonJSONResponseExpected extends true + ? ResponseBody | WretchResponse | null + : ResponseBody | null export type ResourceChangeParams< RequestBody, ResponseBody, RequestQuerySchema extends z.Schema | undefined = undefined, + IsNonJSONResponseExpected extends boolean = false, > = (RequestBody extends z.Schema - ? BodyRequestParams - : FreeBodyRequestParams) & + ? BodyRequestParams + : FreeBodyRequestParams) & (RequestQuerySchema extends z.Schema - ? QueryParams - : FreeQueryParams) + ? QueryParams + : FreeQueryParams) // We don't know which addons Wretch will have, and we don't really care, hence any // eslint-disable-next-line @typescript-eslint/no-explicit-any diff --git a/vitest.config.ts b/vitest.config.ts index 22e65c2..fd01de6 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -7,13 +7,19 @@ export default defineConfig({ globals: true, coverage: { include: ['src/**/*.ts'], - exclude: ['src/**/*.spec.ts', 'src/**/*.test.ts', 'src/index.ts', 'src/types.ts'], - reporter: ['lcov', 'text-summary'], + exclude: [ + 'src/**/*.spec.ts', + 'src/**/*.test.ts', + 'src/index.ts', + 'src/types.ts', + 'src/utils/either.ts', + ], + reporter: ['lcov', 'text'], all: true, thresholds: { lines: 99, functions: 91, - branches: 100, + branches: 99, statements: 99, }, }, From 98e82b264b7ce68ff5528ae42f7e050d9e7e92fa Mon Sep 17 00:00:00 2001 From: Igor Savin Date: Mon, 8 Jul 2024 18:48:31 +0300 Subject: [PATCH 15/15] Fix types, improve coverage, remove dead code --- src/client.test.ts | 42 +++++++++++++++++++++++++++++++++++++++--- src/client.ts | 13 ++++++++++--- src/types.ts | 4 +--- src/utils/bodyUtils.ts | 17 ++++------------- vitest.config.ts | 8 ++++---- 5 files changed, 58 insertions(+), 26 deletions(-) diff --git a/src/client.test.ts b/src/client.test.ts index 9eb293a..3f5ec83 100644 --- a/src/client.test.ts +++ b/src/client.test.ts @@ -817,13 +817,20 @@ describe('frontend-http-client', () => { const response = await sendGet(client, { path: '/', - responseBodySchema: z.any(), + responseBodySchema: z.null(), isEmptyResponseExpected: true, }) + + // This is for checking TS types, we are checking if it infers that the type is not WretchResponse correctly + if (response) { + // @ts-expect-error WretchResponse has this field, ResponseBody does not + expect(response.ok).toBe(true) + } + expect(response).toBe(null) }) - it('returns not json response', async () => { + it('returns non-JSON response', async () => { const client = wretch(mockServer.url) await mockServer.forGet('/').thenReply(200) @@ -838,7 +845,7 @@ describe('frontend-http-client', () => { ) }) - it('returns expected not json response', async () => { + it('returns expected non-JSON response', async () => { const client = wretch(mockServer.url) await mockServer.forGet('/').thenReply(200) @@ -849,6 +856,11 @@ describe('frontend-http-client', () => { isNonJSONResponseExpected: true, }) + // This is for checking TS types, we are checking if it infers the responseBody type as null | WretchResponse correctly + if (responseBody) { + expect(responseBody.ok).toBe(true) + } + expect(responseBody).containSubset({ status: 200, statusText: 'OK', @@ -1071,6 +1083,30 @@ describe('frontend-http-client', () => { expect(response).toBeNull() }) + it('validates query params', async () => { + const client = wretch(mockServer.url) + const testQueryParams = { param1: 'test', param2: 'test' } + + await expect( + sendDelete(client, { + path: '/', + // @ts-expect-error Schema does not match the object + queryParams: testQueryParams, + queryParamsSchema: z.string(), + }), + ).rejects.toThrowErrorMatchingInlineSnapshot(` + [ZodError: [ + { + "code": "invalid_type", + "expected": "string", + "received": "object", + "path": [], + "message": "Expected string, received object" + } + ]] + `) + }) + it('throws if content is expected, but response is empty', async () => { const client = wretch(mockServer.url) diff --git a/src/client.ts b/src/client.ts index 7e672c5..926d970 100644 --- a/src/client.ts +++ b/src/client.ts @@ -1,7 +1,7 @@ import { stringify } from 'fast-querystring' import type { WretchResponse } from 'wretch' import { WretchError } from 'wretch/resolver' -import type { z } from 'zod' +import { z } from 'zod' import type { DeleteParams, @@ -15,6 +15,8 @@ import type { import { tryToResolveJsonBody } from './utils/bodyUtils.js' import { type Either, failure, success, isFailure } from './utils/either.js' +const UNKNOWN_SCHEMA = z.unknown() + function parseRequestBody({ body, requestBodySchema, @@ -258,7 +260,12 @@ export function sendPut< IsNonJSONResponseExpected extends boolean = false, >( wretch: T, - params: ResourceChangeParams, + params: ResourceChangeParams< + RequestBodySchema, + ResponseBody, + RequestQuerySchema, + IsNonJSONResponseExpected + >, ): Promise> { return sendResourceChange(wretch, 'put', params) } @@ -310,7 +317,7 @@ export function sendDelete< const bodyParseResult = await tryToResolveJsonBody( response, params.path, - params.responseBodySchema, + params.responseBodySchema ?? UNKNOWN_SCHEMA, ) if (bodyParseResult.error === 'NOT_JSON') { diff --git a/src/types.ts b/src/types.ts index e717b25..5156b73 100644 --- a/src/types.ts +++ b/src/types.ts @@ -59,9 +59,7 @@ export type FreeDeleteParams = isNonJSONResponseExpected extends true - ? ResponseBody | WretchResponse | null - : ResponseBody | null +> = isNonJSONResponseExpected extends true ? WretchResponse | null : ResponseBody | null export type ResourceChangeParams< RequestBody, diff --git a/src/utils/bodyUtils.ts b/src/utils/bodyUtils.ts index aaa791d..e7b8208 100644 --- a/src/utils/bodyUtils.ts +++ b/src/utils/bodyUtils.ts @@ -1,18 +1,13 @@ import type { WretchResponse } from 'wretch' -import type { ZodError } from 'zod' -import { z } from 'zod' +import type { ZodError, z } from 'zod' import type { Either } from './either.js' import { failure, success } from './either.js' -const ANY_PAYLOAD_SCHEMA = z.any() - -export function tryToResolveJsonBody< - RequestBodySchema extends z.ZodSchema = typeof ANY_PAYLOAD_SCHEMA, ->( +export function tryToResolveJsonBody( response: WretchResponse, path: string, - schema: RequestBodySchema = ANY_PAYLOAD_SCHEMA as unknown as RequestBodySchema, + schema: RequestBodySchema, ): Promise< Either<'NOT_JSON' | 'EMPTY_RESPONSE' | ZodError, z.output> > { @@ -43,13 +38,9 @@ function parseResponseBody({ path, }: { response: ResponseBody - responseBodySchema?: z.ZodSchema + responseBodySchema: z.ZodSchema path: string }): Either { - if (!responseBodySchema) { - return success(response) - } - const result = responseBodySchema.safeParse(response) if (!result.success) { diff --git a/vitest.config.ts b/vitest.config.ts index fd01de6..ff5ea2e 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -17,10 +17,10 @@ export default defineConfig({ reporter: ['lcov', 'text'], all: true, thresholds: { - lines: 99, - functions: 91, - branches: 99, - statements: 99, + lines: 100, + functions: 100, + branches: 100, + statements: 100, }, }, },