Skip to content
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

feature: added support for graphql api querying #49

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 60 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -222,3 +222,63 @@ logger.error('error message', { error: new Error('error') });
```

</details>

## Monday API Client

<details>
<summary>Querying the monday.com GraphQL API seamlessly on behalf of the connected user, or using a provided API token.</summary>

#### initialize

```typescript
import { MondayApiClient } from '@mondaycom/apps-sdk';

const mondayClient = new MondayApiClient();
mondayClient.setToken(token);
mondayClient.setApiVersion('2023-10');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

```

```typescript
import { MondayApiClient } from '@mondaycom/apps-sdk';

const mondayClient = new MondayApiClient(token);
mondayClient.setApiVersion('2023-10');
```

#### API

```typescript
const query = `query {
items(ids: [${itemId}]) {
column_values(ids: ["${columnId}"]) {
text
}
}
}`;

logger.debug(`getApiToGetColumnValue->query:
${query}
`);
const response = await mondayClient.api(query);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

```

```typescript
const query = `mutation ($value: String, $board_id: ID!, $item_id: ID, $column_id: String!){
change_simple_column_value (
board_id: $board_id,
item_id: $item_id,
column_id: $column_id,
value: $value ) {
id
}
}`;
const variables = {
board_id: boardId,
item_id: itemId,
column_id: columnId,
value: value.toString(),
};
const response = await await mondayClient.api(query, { variables });
```

</details>
4 changes: 3 additions & 1 deletion lib/index.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { EnvironmentVariablesManager } from 'lib/environment-variables-manager';
import { Logger } from 'lib/logger';
import { MondayApiClient } from 'lib/monday-api-client';
import { Queue } from 'lib/queue';
import { SecureStorage } from 'lib/secure-storage';
import { Period, Storage } from 'lib/storage';
Expand All @@ -10,5 +11,6 @@ export {
Period,
EnvironmentVariablesManager,
Logger,
Queue
Queue,
MondayApiClient
};
2 changes: 1 addition & 1 deletion lib/minimal-package.ts
Original file line number Diff line number Diff line change
@@ -1 +1 @@
export default { name: '@mondaycom/apps-sdk', version: '2.2.0' };
export default { name: '@mondaycom/apps-sdk', version: '2.3.0' };
4 changes: 4 additions & 0 deletions lib/monday-api-client/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
import { MondayApiClient } from './monday-api-client'
export {
MondayApiClient
};
78 changes: 78 additions & 0 deletions lib/monday-api-client/monday-api-client.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
import {BadRequestError, InternalServerError} from 'errors/apps-sdk-error';
import {Token} from 'types/general';
import {isDefined} from 'types/guards';
Comment on lines +1 to +3
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please run your linter?

import {
APIData,
APIFetchResponse,
APIOptions,
APIResponse,
APIVersion,
IMondayApiClient
} from 'types/monday-api-client';
import {fetchWrapper} from 'utils/fetch-wrapper';
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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can call it simply ApiClient, no need for the Monday prefix

private token?: Token;

private apiVersion?: APIVersion;

constructor(token?: Token) {
this.token = token;
}

setToken(token: Token) {
this.token = token;
}

setApiVersion(version: APIVersion){
this.apiVersion = version
}

async api(query: string, options: APIOptions = {}) {
if (!this.token) {
logger.warn('[mondayApiClientFetch] Error Token is missing');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be a warn level log if it states there was an error

throw new BadRequestError('Token is missing');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure BadRequest is the way to go here (api wise) this should be Unauthorized, no?

}

const params: APIData = { query, variables: options.variables };
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having variables as undefined is ok? Should we transfer it if this is the scenario?

const apiVersion = options.apiVersion || this.apiVersion;

const fetchObj = {
headers: {
'Content-Type': 'application/json',
...(this.token && { 'Authorization': this.token }),
...(apiVersion && { 'API-Version': apiVersion })
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not an X header??

},
method: 'POST',
body: params ? JSON.stringify(params || {}) : undefined
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
body: params ? JSON.stringify(params || {}) : undefined
body: JSON.stringify(params || {})

Params is always defined, there is no reason to have this ternary here (look at line 39)

};

let result: APIFetchResponse | undefined;
try {
result = await fetchWrapper<APIResponse>(API_URL, fetchObj);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this address (API_URL) extracted to a constant? it is used only once.
Don't you think it will be more readable if it will be written here in an hard coded manner?

} catch (error: unknown) {
logger.error('[mondayApiClientFetch] Unexpected error occurred while communicating with secure storage', { error: error as Error });
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should extract mondayApiClientFetch as a general constant for this file and reuse it in the logs

throw new InternalServerError('An issue occurred while accessing secure storage');
}


if (!isDefined(result)) {
throw new InternalServerError('some thing went wrong when communicating with mondayApiClientFetch');
}

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}`);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is related to secure storage here 😅

throw new BadRequestError(errorMessage);
}

if (!isDefined(result.data)) {
throw new InternalServerError('some thing went wrong when communicating with mondayApiClientFetch data filed returned empty');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
throw new InternalServerError('some thing went wrong when communicating with mondayApiClientFetch data filed returned empty');
throw new InternalServerError('some thing went wrong when communicating with mondayApiClientFetch data field returned empty');

}

const { data, account_id } = result;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we commit on returning account_id as snake case?
And a general question - Why do we return accountId here?

return { data, account_id }
}
}
3 changes: 2 additions & 1 deletion lib/storage/base-storage.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import {BadRequestError, InternalServerError} from 'errors/apps-sdk-error';
import {RequestOptions} from 'types/fetch';
import {Token} from 'types/general';
Comment on lines 2 to +3
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lint

import {isDefined} from 'types/guards';
import {Options, Token} from 'types/storage';
import {Options} from 'types/storage';
import {fetchWrapper} from 'utils/fetch-wrapper';
import {Logger} from 'utils/logger';

Expand Down
2 changes: 2 additions & 0 deletions lib/types/general.ts
Original file line number Diff line number Diff line change
@@ -1 +1,3 @@
export type JsonValue = string | number | boolean | null | Array<JsonValue> | { [key: string]: JsonValue };

export type Token = string;
60 changes: 60 additions & 0 deletions lib/types/monday-api-client.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@

export type APIOptions = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DorShakedMonday in that case they can simply create another ApiClient with the right version and use it for their deprecated methods

/**
* Access token for the API
* If not set, will use the credentials of the current user (client only)
*/
token?: string;

/**
* An object containing GraphQL query variables
*/
variables?: object;

/**
* A string specifying which version of the API should be used
* If not set, will use the current API version
*/
apiVersion?: string;
}

export type APIData = {
query: string;
variables?: object;
}

export type APIVersion = string;

export type APIResponse = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's not always the case, what about errors?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look at lines 35 ~ 37.
@ShayElkana Why are these types separated in this manner? Developers can get errors, no?

data?: {
data: object
},
account_id: number
}

export type APIFetchResponse = {
errors?: Array<{ message: string }>,
} & APIResponse;

export type IMondayApiClient = {

/**
* Used for querying the monday.com GraphQL API seamlessly on behalf of the connected user, or using a provided API token.
* For more information about the GraphQL API and all queries and mutations possible, read the [API Documentation](https://monday.com/developers/v2)
* @param query A [GraphQL](https://graphql.org/) query, can be either a query (retrieval operation) or a mutation (creation/update/deletion operation).
* 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>,
Copy link
Contributor

@gregra81 gregra81 Jan 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>>
}

/**
* Instead of passing the API token to the `api()` method on each request, you can set the API token once using:
* @param token Access token for the API
*/
setToken(token: string): void;

/**
* Allows to set the API version for future requests.
* @param version A string specifying which version of the API should be used
*/
setApiVersion(version: string): void;
}
2 changes: 1 addition & 1 deletion lib/types/storage.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
export type Token = string;

export enum Period {
DAILY='DAILY',
MONTHLY='MONTHLY',
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@mondaycom/apps-sdk",
"version": "2.2.0",
"version": "2.3.0",
"description": "monday apps SDK for NodeJS",
"main": "./dist/cjs/index.js",
"module": "./dist/esm/index.js",
Expand Down
Loading