Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: return 404 if Storipress API return 404 #321

Conversation

SidStraw
Copy link
Contributor

https://storipress-media.atlassian.net/browse/SPMVP-7009

  1. Return a 404 when the resource is not found.
  2. Return the API error code and the original error when there is a payload error.

@SidStraw SidStraw requested a review from DanSnow January 29, 2024 01:27
listAll,
getOne,
listHash,
}: DefinePayloadHandlerInput<T>): any {
return defineSnapshotHandler(async (name, event) => {
async function handleListAll(bypassCache: boolean): Promise<T[] | KarbonErrorMeta> {
return await listAll(bypassCache).catch((error: KarbonError | ApolloError) => {
const networkError = (error as ApolloError)?.networkError as { statusCode: number }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❔ 這邊會如何處理 typesense 的 error 呢?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TypesenseError 也是透過 httpStatus 的屬性來讀取狀態碼的
已經有修正成使用 TypesenseError 的 type

回傳的資料中也有包含原始的 Error 物件提供錯誤判斷

這邊不是直接回傳 Error 是因為如果回傳 Error 物件作為 Response 的話,Nuxt 會無視 setResponseStatus 的設置回傳 500

export interface KarbonErrorMeta {
payloadScope?: PayloadScope
function?: string
symbol: typeof KarbonErrorSymbol
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 考量到這邊可能是在 internal.ts export ,也可能在其它地方 export 而導致使用的 symbol 是不同的版本,建議考慮用其它基礎型態來識別 meta ,比如像 Vue 的作法 https://github.com/vuejs/core/blob/d276a4f3e914aaccc291f7b2513e5d978919d0f9/packages/reactivity/src/ref.ts#L382

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已調整

@SidStraw SidStraw requested a review from DanSnow January 29, 2024 07:26
@DanSnow DanSnow merged commit 957f3e7 into main Jan 29, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants