feature: added support for graphql api querying #49
Conversation
There was a problem hiding this comment.
My main comment here, is that I think we should create a new package called
@mondaycom/api and reuse it here and in monday-sdk-js. The reason is that during this year we are going to enhance api sdk to support popular methods, such as createItem, etc and I want both SDKs to enjoy it
| import { Logger } from 'utils/logger'; | ||
| const logger = new Logger('MondayApiClient', { mondayInternal: true }); | ||
| const API_URL = 'https://api.monday.com/v2/'; | ||
| export class MondayApiClient implements IMondayApiClient { |
There was a problem hiding this comment.
we can call it simply ApiClient, no need for the Monday prefix
| @@ -0,0 +1,60 @@ | |||
|
|
|||
| export type APIOptions = { | |||
There was a problem hiding this comment.
IMO we don't need it, as it allows the developer to use different tokens and apiVersions for different api calls using the same client. This is not standard in the industry. Regarding the variables param - simple move it to the api function and maybe change its type to Record<string, any>
There was a problem hiding this comment.
This may make the life of developers a little bit harder. If they have one mutation that they don't know how to deprecate yet (move to the new version) and they know how to move the rest of their stuff - Without this option, They will "have" to stay on the old version instead of moving and staying in the old version only for this mutation
There was a problem hiding this comment.
@DorShakedMonday in that case they can simply create another ApiClient with the right version and use it for their deprecated methods
|
|
||
| export type APIVersion = string; | ||
|
|
||
| export type APIResponse = { |
There was a problem hiding this comment.
that's not always the case, what about errors?
There was a problem hiding this comment.
Look at lines 35 ~ 37.
@ShayElkana Why are these types separated in this manner? Developers can get errors, no?
| * Placeholders may be used, which will be substituted by the variables object passed within the options. | ||
| * @param options | ||
| */ | ||
| api: (query: string, options: APIOptions) => Promise<APIResponse>, |
There was a problem hiding this comment.
use generics to allow the developer to get the type, i.e.:
export type APIResponse<T> = {
data: T,
account_id: number
}
export type APIFetchResponse<T> = {
errors?: Array<{ message: string }>,
} & APIResponse<T>;
export type IMondayApiClient = {
api: <T = object>(query: string) => Promise<APIFetchResponse<T>>
}
|
|
||
| const mondayClient = new MondayApiClient(); | ||
| mondayClient.setToken(token); | ||
| mondayClient.setApiVersion('2023-10'); |
There was a problem hiding this comment.
Question out of gap in knowledge. Is this the best practice we dictate to our developers? to hardcode the api version? there is no env variable or something we suggest using/creating to manage this?
| logger.debug(`getApiToGetColumnValue->query: | ||
| ${query} | ||
| `); | ||
| const response = await mondayClient.api(query); |
There was a problem hiding this comment.
Why don't you use variables here like in the mutation? this would look like we don't recommend using variables in queries (like in lines 265~281)
| import {BadRequestError, InternalServerError} from 'errors/apps-sdk-error'; | ||
| import {Token} from 'types/general'; | ||
| import {isDefined} from 'types/guards'; |
There was a problem hiding this comment.
Can you please run your linter?
|
|
||
| async api(query: string, options: APIOptions = {}) { | ||
| if (!this.token) { | ||
| logger.warn('[mondayApiClientFetch] Error Token is missing'); |
There was a problem hiding this comment.
This should not be a warn level log if it states there was an error
| async api(query: string, options: APIOptions = {}) { | ||
| if (!this.token) { | ||
| logger.warn('[mondayApiClientFetch] Error Token is missing'); | ||
| throw new BadRequestError('Token is missing'); |
There was a problem hiding this comment.
I am not sure BadRequest is the way to go here (api wise) this should be Unauthorized, no?
|
|
||
| if (isDefined(result.errors)) { | ||
| const errorMessage = result.errors.map(e=>e?.message).join(', '); | ||
| logger.error(`[mondayApiClientFetch] Errors occurred while communicating with secure storage.\nErrors: ${errorMessage}`); |
There was a problem hiding this comment.
I don't think this is related to secure storage here 😅
| throw new InternalServerError('some thing went wrong when communicating with mondayApiClientFetch data filed returned empty'); | ||
| } | ||
|
|
||
| const { data, account_id } = result; |
There was a problem hiding this comment.
Do we commit on returning account_id as snake case?
And a general question - Why do we return accountId here?
| import {RequestOptions} from 'types/fetch'; | ||
| import {Token} from 'types/general'; |
| @@ -0,0 +1,60 @@ | |||
|
|
|||
| export type APIOptions = { | |||
There was a problem hiding this comment.
This may make the life of developers a little bit harder. If they have one mutation that they don't know how to deprecate yet (move to the new version) and they know how to move the rest of their stuff - Without this option, They will "have" to stay on the old version instead of moving and staying in the old version only for this mutation
|
|
||
| export type APIVersion = string; | ||
|
|
||
| export type APIResponse = { |
There was a problem hiding this comment.
Look at lines 35 ~ 37.
@ShayElkana Why are these types separated in this manner? Developers can get errors, no?
https://monday.monday.com/boards/3670992828/views/83352829