[mds-agency] Removing version param from agency payload to conform with general v… #331
Conversation
…ersioninig policy.
| @@ -471,8 +460,7 @@ export const readStop = async (req: AgencyApiRequest, res: AgencyReadStopRespons | |||
| if (!recorded_stop) { | |||
| return res.status(404).send({ error: new NotFoundError('Stop not found') }) | |||
| } | |||
| const { version } = res.locals | |||
| res.status(200).send({ version, ...recorded_stop }) | |||
| res.status(200).send({ ...recorded_stop }) | |||
There was a problem hiding this comment.
.send({ ...recorded_stop }) could just be .send(recorded_stop)
| return res.status(201).send({ | ||
| version | ||
| }) | ||
| return res.status(201).send({}) |
There was a problem hiding this comment.
Does res.sendStatus(201) work or does res.status(201).send({}) result in different content type headers?
There was a problem hiding this comment.
It sends a different header. It takes out the charset: utf-8.
| @@ -31,7 +31,7 @@ export type ApiQuery<Q1 extends string, Q2 extends string[] = never> = { | |||
|
|
|||
| export interface ApiResponse<L = unknown, B = unknown> | |||
| extends express.Response< | |||
| B | { error: unknown; error_description?: string; error_details?: string[] } | { errors: unknown[] } | {} | |||
| B | { error: unknown; error_description?: string; error_details?: string[] } | { errors: unknown[] } | |||
There was a problem hiding this comment.
Would it make sense to make this it's own interface
interface ApiErrorReponse {
error: unknown
error_description?: string
error_details?: string[]
}And make the AprResponse <B | ApiErrorResponse | { errors: ApiErrorReponse[] }
| @@ -149,8 +147,7 @@ export const getVehiclesByProvider = async (req: AgencyApiRequest, res: AgencyGe | |||
|
|
|||
| try { | |||
| const response = await getVehicles(skip, take, url, req.query, provider_id) | |||
| const { version } = res.locals | |||
| return res.status(200).send({ version, ...response }) | |||
| return res.status(200).send({ ...response }) | |||
There was a problem hiding this comment.
Still not sure the new object syntax is required in all of these places { ...response } and response being equivalent and all.
There was a problem hiding this comment.
It's the difference between { response: {...}} and { "vehicles": [ ... ] "links": { "first": "https://...", "last": "https://...", "prev": "https://...", "next": "https://..." }}, and the former is not in the spec.
There was a problem hiding this comment.
There is no difference between .send({...response}) and .send(response) ... note that I didn't suggest .send({ response })
There was a problem hiding this comment.
I agree with Michael, .send({ ...response }) and .send(response) are the same AFAIK
packages/mds-agency/types.ts
Outdated
| @@ -11,8 +11,7 @@ export type AgencyApiRequest<P extends Params = ParamsDictionary> = ApiRequest<P | |||
|
|
|||
| export type AgencyApiAccessTokenScopes = 'admin:all' | 'vehicles:read' | |||
|
|
|||
| export type AgencyApiResponse<TBody = any> = ApiVersionedResponse< | |||
| AGENCY_API_SUPPORTED_VERSION, | |||
| export type AgencyApiResponse<TBody = any> = ApiResponse< | |||
There was a problem hiding this comment.
I think you can remove the = any (and thus the eslint warning) since a type is always specified below. Also, unknown is preferable to any for don't care scenarios.
…ersioninig policy.
PR Checklist
[mds-db] Add PG env var,[config] Fix eslint configImpacts