feat(cms/api): add basic API client #238
Conversation
Varun-Sethu
force-pushed
the
varun/APIClient
branch
from
b03b0e8 to
2da112b
Compare
Varun-Sethu
force-pushed
the
varun/APIClient
branch
7 times, most recently
from
ad8b3b8 to
91d59fd
Compare
Varun-Sethu
force-pushed
the
varun/APIClient
branch
from
91d59fd to
e34370d
Compare
lhjt
changed the title
lhjt
changed the title
| return response.ok | ||
| ? (await response.json()).Response as ResponseType | ||
| : (await response.json()) as APIError; |
There was a problem hiding this comment.
what if response.json() fails? gotta double abstract error handling 😳
| import { CreateFilesystemEntryResponse, FilesystemEntry } from "./types/filesystem"; | ||
| import { APIError, EmptyAPIResponse } from "./types/general"; |
There was a problem hiding this comment.
thoughts on using import type since none of these are being used as values?
related: https://typescript-eslint.io/rules/consistent-type-imports/
There was a problem hiding this comment.
oooh I didnt even know this was a thing 😎
| // Only interface with the BE FS APIs via this class | ||
| export class FilesystemAPI { | ||
| // GetEntityInfo retrieves all entity information for an FS entity given its ID | ||
| public static GetEntityInfo = (EntityID: string): Promise<FilesystemEntry | APIError> => |
There was a problem hiding this comment.
thoughts on abstracting these return types since they're all of the same format (Promise<T | APIError>)?
There was a problem hiding this comment.
IMO I feel like that's kinda unnecessary, its a single line type
|
|
||
| // SendGetRequest is a small helper functions for sending get request and wrapping the response in an appropriate type | ||
| static async SendGetRequest<ResponseType> (url: string): Promise<ResponseType | APIError> { | ||
| const response = await fetch(`${API_URL}${url}`); |
There was a problem hiding this comment.
thoughts on catching errors here as well (e.g., invalid url or dns lookup failures)? (ayo triple abstraction??)
| export const hasFieldOfType = (o: any, fieldName: string, type: string): boolean => | ||
| fieldName in o && (typeof o[fieldName]) === type; |
There was a problem hiding this comment.
not sure if it makes more sense to use fieldType here since you're using fieldName as well - i got really confused here for a minute because i thought type was some sort of built-in, or some typescript magic was going on here 😳
There was a problem hiding this comment.
yeah thats a good point, shall rename to fieldType
| hasFieldOfType(o, "EntityName", "string") && | ||
| hasFieldOfType(o, "IsDocument", "boolean") && | ||
| hasFieldOfType(o, "Parent", "string") && | ||
| hasFieldOfType(o, "Children", typeof []) && |
There was a problem hiding this comment.
this isn't very reliable since typeof [] is just "object" (javascript moment) - you probably want to use Array.isArray here
| run: | | ||
| GO_MOD=go.mod docker compose --env-file=./config/ci.env.dev up --wait --build backend db staging_db frontend | ||
| - name: Backend Logs | ||
| run: | | ||
| docker logs go_backend | ||
| docker logs pg_container | ||
| docker ps | ||
| docker logs go_backend | ||
| # docker exec frontend curl http://backend:8080/api/filesystem/info | ||
| # docker exec frontend npm test |
There was a problem hiding this comment.
Uh sorry so just to clarify but how is the docker compose build part running any sort of tests? And should should the lines at the bottom be commented out?
cc @sachk
There was a problem hiding this comment.
smh i pointed this out and now jared is STEALING the credit - more ABUSE by csesoc execs :(
There was a problem hiding this comment.
The bottom two lines were commented for testing reasons (they should not be commented out), the entire PR is still blocked by the weird networking issue 😢
Varun-Sethu
requested review from
lauraw0,
jamest0 and
hdphuong
as code owners
Co-authored-by: Michael Vo <[email protected]>
Co-authored-by: Michael Vo <[email protected]>
Co-authored-by: Michael Vo <[email protected]>
Co-authored-by: Michael Vo <[email protected]>
Hmm seems like its worth doing although u probably want to talk to the FE team abt that |
|
|
||
| // publish it | ||
| const publishResp = await FilesystemAPI.RenameEntity(newEntityId, "docMcStuffins"); | ||
| expect(IsEmptyApiResponse(publishResp), 'Expected deletion response to be assignable to empty'); |
|
|
||
| // Create a document | ||
| const newDocument = await FilesystemAPI.CreateDocument("ebic document of truth", root.EntityID); | ||
| expect(IsCreateFilesystemEntryResponse(newDocument), "Expected CreateDocument response to be assignable to CreateFilesystemEntryResponse").toBe(true); |
There was a problem hiding this comment.
shouldn’t createDocument take in 3 arguments?
| FilesystemAPI.SendGetRequest<FilesystemEntry>(`/api/filesystem/info`); | ||
|
|
||
| // CreateEntity constructs an editable un-published filesystem entry | ||
| public static CreateDocument = (Name: string, ParentID: string, ownerGroup = 1): Promise<CreateFilesystemEntryResponse | APIError> => |
There was a problem hiding this comment.
nitpicking but it should be CreateDocument not CreateEntity in the comment
Also, some filesystem endpoints are missing, is that intentional 👀
| @@ -30,5 +30,7 @@ func main() { | |||
| handler := cors.Default().Handler(mux) | |||
| handler = c.Handler(handler) | |||
|
|
|||
| log.Print("CMS Go backend starting on port :8080 :D.") | |||
| log.Print("Amongus.") | |||
PR adds a really small and basic API client for interfacing with the API, the client provides strict type definitions to make refactoring easier if we ever change the API contract between FE and BE (also serves as an implicit source of documentation)