-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add R API container to dev dependencies script, and consume R API from app #10
Changes from 5 commits
1587ce4
704d4b7
985a75f
3c98f7c
ee643a0
18cb9e4
d4c216e
e8567db
e8ffe19
7ee6a28
d14a0eb
7669aa7
f187eeb
1897a64
77e31a4
5aa5870
234bac2
418189f
94612e8
a978237
b2ce44c
2bff65b
fb2bf48
7f44bc5
776d3a8
1eabcfc
1c9e574
276e91e
76bef3b
0357db4
68a2047
e1921c8
2c94900
1a4715e
ee66623
18d6ea1
11801e2
871bb3d
43d47b0
d45d437
c70b3aa
4d8f286
54aa577
6e0ed26
1a82a3b
a2bfe29
c862e0f
5d875f7
5de1f22
6cfc4bd
0701f44
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,19 +1,13 @@ | ||
import { getVersionData } from "@/server/handlers/versions"; | ||
import { errorMessage } from "@/server/utils/helpers"; | ||
import type { VersionData } from "@/types/apiResponseTypes"; | ||
import { defineEventHandlerWithErrors } from "@/server/utils/defineEventHandlerWithErrors"; | ||
import type { VersionDataResponse } from "@/types/daedalusApiResponseTypes"; | ||
|
||
export default defineEventHandler(async (event): Promise<VersionData> => { | ||
// Delegate to handler function getVersionData so that the logic can be unit-tested. | ||
const versionDataResponse = await getVersionData(event); | ||
export default defineEventHandlerWithErrors( | ||
// TODO: Consider cacheing this server-side https://nitro.unjs.io/guide/cache | ||
defineEventHandler(async (event): Promise<VersionDataResponse> => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a bit surprised that you're still invoking There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I found a way around this, here is the commit: b029800 It's not yet a commit that lives in a PR. |
||
// Delegate to getVersionData so that the logic can be unit-tested. | ||
const versionDataResponse = await getVersionData(event); | ||
|
||
if (versionDataResponse.errors || !versionDataResponse.data) { | ||
throw createError({ | ||
statusCode: versionDataResponse.statusCode, | ||
statusMessage: versionDataResponse.statusText, | ||
message: errorMessage(versionDataResponse.errors), | ||
data: versionDataResponse.errors, | ||
}); | ||
} else { | ||
return versionDataResponse.data; | ||
} | ||
}); | ||
return versionDataResponse; | ||
}), | ||
); | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
import type { EventHandler, EventHandlerRequest } from "h3"; | ||
import type { ApiResponse } from "@/types/daedalusApiResponseTypes"; | ||
|
||
export const defineEventHandlerWithErrors = <T extends EventHandlerRequest, D>( | ||
handler: EventHandler<T, D>, | ||
): EventHandler<T, D> => | ||
defineEventHandler<T>(async (event) => { | ||
const response = await handler(event) as ApiResponse; | ||
|
||
if (response.errors || !response.data) { | ||
throw createError({ | ||
statusCode: response.statusCode, | ||
statusMessage: response.statusText, | ||
message: errorMessage(response.errors), | ||
data: response.errors, | ||
}); | ||
} else { | ||
return response.data; | ||
} | ||
}); | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,8 @@ | ||
import type { ApiError } from "@/types/apiResponseTypes"; | ||
import type { ApiError } from "@/types/daedalusApiResponseTypes"; | ||
|
||
// Convert list of error objects to string | ||
export const errorMessage = (errors: Array<ApiError> | null) => { | ||
return errors?.map((apiError) => { | ||
return [apiError.error, apiError.detail].join(": "); | ||
}).join(", "); | ||
}; | ||
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.
It seems likely that this pattern will be repeated a lot, so we'll probably eventually want a generic typs something like
so then this line would become
const { data: versionData } = useFetch("/api/versions") as APIResponse<VersionData>;
..but then I suspect maybe we'll also eventually have some sort of
useAPI
which wraps the rawuseFetch
and just gives back the data you're interested in, and helps with error handling. So probably not one for this PR.UPDATE: I see below that you've got
ServerApiResponse
, but I think that refers to responses from the R API?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.
ServerApiResponse
was named to try and distinguish it from the R API, so that'll have to be re-named... if we simply call itApiResponse
might that actually be clearer?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.
Though
ServerApiResponse
's purpose differs from what you're talking about -ServerApiResponse
(well, types extended from it) are to be used inside the API routes to dictate the format of the response's data, rather than to be used wrapping a useFetch.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.
Yeah, exactly. I think probably the response types from the R API (used in the web app backend) and the types from the web app backend (used in the front end) should probably live in different modules, and maybe a "shared" module for ones that happen to be the same, with whatever generic classes are useful in the two contexts. Maybe
ServerApiResponse
andRApiResponse
(I thinkDaedalusApiResponse
would be more correct, but very verbose, and you already have rApi module...)