feat(backend,frontend,client,common): implementation#1
Conversation
melancholiai
commented
Jan 23, 2024
melancholiai
commented
| Question | Answer |
|---|---|
| Bug fix | ✖ |
| New feature | ✔ |
| Breaking change | ✖ |
| Deprecations | ✖ |
| Documentation | ✔ |
| Tests added | ✔ |
| Chore | ✖ |
melancholiai
changed the title
| fetch: { | ||
| method: 'GET', | ||
| headers: { | ||
| 'x-api-key': `eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJNYXBDb2xvbmllc0RldiIsImlhdCI6MTUxNjIzOTAyMiwiZCI6WyJyYXN0ZXIiLCJyYXN0ZXJXbXMiLCJyYXN0ZXJFeHBvcnQiLCJkZW0iLCJ2ZWN0b3IiLCIzZCJdfQ.GvTQ_yLjnioxxFrNgGQiuarhJxLpe8AhTTtrWE3LHoUED48CFKBEOfKqOyEWSDVZjx1jHkDvZAL1iyEvi5FHNys7UBRXCiJvVlG-muJZ6ycS9PGKauzL-eggXqTqGsXh4FBkqvHUEElXEnu7ARsMCm5eIC66U2i_eHFU3PLcOc67qJvS1IQjAI2oj9Pd5mGaI_HlDaf3B4PFOb0AHdY-r_MDGwck3asm1G_InVzsvCXt36vImyn1Z4HYaN4YiDfaMLBF0-GGrlLE84PObzGGtt66EIuQ4OneEZSzoQNusBt5-SFs0EQXsfsDc_RMRTz3DZseqkNIKiXEsEBBPjMr7w`, |
There was a problem hiding this comment.
is this a real token ? it shouldnt be here
| @@ -0,0 +1,13 @@ | |||
| export class TileDetailsNotFoundError extends Error { | |||
There was a problem hiding this comment.
Suggestion:
I think you should implement HttpError object from @map-colonies/error-express-handler on this error.
Then, add public readonly status = httpStatus.NOT_FOUND; - instead of catching the error and setting the returned status code manually.
There was a problem hiding this comment.
I disagree.
TileDetailsNotFoundError is being used in TileDetailsManager which shouldn't be decoupled to an http server. thus implementing errors with http properties would not be best practice.
for example, this becomes handy when we're unit testing this function and are aware of the exact error that was thrown in the relevant case.
|
|
||
| const tilesDetails = result.documents.map((document) => document.value[0]); | ||
|
|
||
| return tilesDetails as unknown as TileDetails[]; |
There was a problem hiding this comment.
Is there a way to avoid using 'as unkown as x'? I think it's not type-safe
There was a problem hiding this comment.
sadly no, the return type of redis.ft.search isn't type aware of the documents and they might be of a primitive type.
you cannot cast from a custom to a primitive without erasing the type first - casting to unknown erases the type checking.
|
|
||
| const keys = await this.redis.keys(`${REDIS_KITS_HASH_PREFIX}:*`); | ||
|
|
||
| const kits = (await Promise.all(keys.map(async (key) => this.redis.hGetAll(key)))) as KitMetadata[]; |
There was a problem hiding this comment.
Question:
If one fetch fail, do you want this promise to totally reject?
Else, I think you need Promise.allSettled.
There was a problem hiding this comment.
the client is determining its logic by the provided result. an incomplete result might lead to a client mistakes.
if the server couldn't provide the "whole picture" the request should fail.
| @@ -0,0 +1,6 @@ | |||
| export class KitAlreadyExistsError extends Error { | |||
There was a problem hiding this comment.
Also here I suggest you to implement HttpError object from @map-colonies/error-express-handler.
| export const bboxToWktPolygon = (bbox: BoundingBox): string => { | ||
| const { west, south, east, north } = bbox; | ||
| return `POLYGON ((${west} ${north}, ${west} ${south}, ${east} ${south}, ${east} ${north}, ${west} ${north}))`; | ||
| }; |
There was a problem hiding this comment.
It's just me but I would destructure the bbox in the parameter and remove the return statement like in keyfy function.
There was a problem hiding this comment.
it's just a styling preference. I find it more readable
| const { logger, ...config } = options; | ||
| this.logger = logger; | ||
| this.config = config; | ||
| this.axios = axios.create({ timeout: options.timeout }); |
There was a problem hiding this comment.
I think the backend is going to be protected with an api-key. If so, you should add here the relevant header (or support it if you get it as part of the options).
| useEffect(() => { | ||
| async function kitsFetch(): Promise<void> { | ||
| try { | ||
| const [result, duration] = await timerify<Awaited<ReturnType<typeof client.getKits>>, never[]>(client.getKits.bind(client)); | ||
| kits = result; | ||
| setStatsTable((prevStatsTable) => { | ||
| const nextHttpStat = calcHttpStat(prevStatsTable.httpInvokes.kits, duration); | ||
| return { ...prevStatsTable, httpInvokes: { ...prevStatsTable.httpInvokes, kits: nextHttpStat } }; | ||
| }); | ||
| } catch (err) { | ||
| logger.error({ msg: 'error fetching kits', err }); | ||
| enqueueSnackbar(JSON.stringify(err), { variant: 'error' }); | ||
| } | ||
| } | ||
| void kitsFetch(); | ||
| setIntervalAsync(kitsFetch, FETCH_KITS_INTERVAL); | ||
| }, []); |
There was a problem hiding this comment.
You should add cleanup for the setInterval if this component will unmount.
| } | ||
| } | ||
| void kitsFetch(); | ||
| setIntervalAsync(kitsFetch, FETCH_KITS_INTERVAL); |
There was a problem hiding this comment.
Personally I prefer CRON over intervals as it makes it more predictable.
There was a problem hiding this comment.
using an async function thus implementation is done with set-interval-async
3 participants
