Skip to content

Commit 4b5b709

Browse files
setchysetchy
committed
introduce client builder and caches for rest and graphql
Signed-off-by: Adam Setch <adam.setch@outlook.com>
1 parent 1eba293 commit 4b5b709

9 files changed

Lines changed: 168 additions & 69 deletions

File tree

‎src/renderer/types.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,8 @@ export type Status = 'loading' | 'success' | 'error';
3737

3838
export type Percentage = Branded<number, 'Percentage'>;
3939

40+
export type AccountUUID = Branded<string, 'AccountUUID'>;
41+
4042
export interface Account {
4143
method: AuthMethod;
4244
platform: PlatformType;

‎src/renderer/utils/api/client.ts‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ import { getNumberFromUrl } from './utils';
4646
export async function headNotifications(
4747
account: Account,
4848
): Promise<HeadNotificationsResponse> {
49-
const octokit = await createOctokitClient(account);
49+
const octokit = await createOctokitClient(account, 'rest');
5050

5151
await octokit.rest.activity.listNotificationsForAuthenticatedUser({
5252
per_page: 1,
@@ -62,7 +62,7 @@ export async function listNotificationsForAuthenticatedUser(
6262
account: Account,
6363
settings: SettingsState,
6464
): Promise<ListNotificationsForAuthenticatedUserResponse> {
65-
const octokit = await createOctokitClient(account);
65+
const octokit = await createOctokitClient(account, 'rest');
6666

6767
if (settings.fetchAllNotifications) {
6868
// Fetch all pages using Octokit's pagination
@@ -103,7 +103,7 @@ export async function markNotificationThreadAsRead(
103103
account: Account,
104104
threadId: string,
105105
): Promise<MarkNotificationThreadAsReadResponse> {
106-
const octokit = await createOctokitClient(account);
106+
const octokit = await createOctokitClient(account, 'rest');
107107

108108
const response = await octokit.rest.activity.markThreadAsRead({
109109
thread_id: Number(threadId),
@@ -124,7 +124,7 @@ export async function markNotificationThreadAsDone(
124124
account: Account,
125125
threadId: string,
126126
): Promise<MarkNotificationThreadAsDoneResponse> {
127-
const octokit = await createOctokitClient(account);
127+
const octokit = await createOctokitClient(account, 'rest');
128128

129129
const response = await octokit.rest.activity.markThreadAsDone({
130130
thread_id: Number(threadId),
@@ -142,7 +142,7 @@ export async function ignoreNotificationThreadSubscription(
142142
account: Account,
143143
threadId: string,
144144
): Promise<IgnoreNotificationThreadSubscriptionResponse> {
145-
const octokit = await createOctokitClient(account);
145+
const octokit = await createOctokitClient(account, 'rest');
146146

147147
const response = await octokit.rest.activity.setThreadSubscription({
148148
thread_id: Number(threadId),
@@ -205,9 +205,9 @@ async function followUrl<TResult>(
205205
account: Account,
206206
url: Link,
207207
): Promise<TResult> {
208-
const octokit = await createOctokitClient(account);
208+
const octokit = await createOctokitClient(account, 'rest');
209209

210-
// Perform a generic GET request using Octokit's request method
210+
// Perform a generic GET request using Octokit's request method and cast the response type
211211
const response = await octokit.request('GET {+url}', {
212212
url: url,
213213
});
@@ -221,7 +221,7 @@ async function followUrl<TResult>(
221221
export async function fetchAuthenticatedUserDetails(
222222
account: Account,
223223
): Promise<OctokitResponse<GetAuthenticatedUserResponse>> {
224-
const octokit = await createOctokitClient(account);
224+
const octokit = await createOctokitClient(account, 'rest');
225225

226226
const response = await octokit.rest.users.getAuthenticated();
227227

‎src/renderer/utils/api/octokit.test.ts‎

Lines changed: 95 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ import {
33
mockGitHubCloudAccount,
44
mockGitHubEnterpriseServerAccount,
55
} from '../../__mocks__/account-mocks';
6-
import { mockToken } from '../../__mocks__/state-mocks';
76

87
import * as comms from '../comms';
98
import { clearOctokitClientCache, createOctokitClient } from './octokit';
@@ -24,43 +23,99 @@ describe('renderer/utils/api/octokit.ts', () => {
2423
});
2524

2625
describe('createOctokitClient', () => {
27-
it('should create octokit client for GitHub Cloud', async () => {
26+
it('should create octokit rest client for GitHub Cloud', async () => {
2827
const getGitHubAPIBaseUrlSpy = jest.spyOn(utils, 'getGitHubAPIBaseUrl');
2928
getGitHubAPIBaseUrlSpy.mockReturnValue(
3029
new URL('https://api.github.com/'),
3130
);
3231

33-
const octokit = await createOctokitClient(mockGitHubCloudAccount);
32+
const octokit = await createOctokitClient(mockGitHubCloudAccount, 'rest');
3433

35-
expect(getGitHubAPIBaseUrlSpy).toHaveBeenCalledWith('github.com');
34+
expect(getGitHubAPIBaseUrlSpy).toHaveBeenCalledWith('github.com', 'rest');
3635
expect(octokit).toBeDefined();
37-
expect(mockDecryptValue).toHaveBeenCalledWith(mockToken);
36+
expect(mockDecryptValue).toHaveBeenCalledWith(
37+
mockGitHubCloudAccount.token,
38+
);
39+
});
40+
41+
it('should create octokit graphql client for GitHub Cloud', async () => {
42+
const getGitHubAPIBaseUrlSpy = jest.spyOn(utils, 'getGitHubAPIBaseUrl');
43+
getGitHubAPIBaseUrlSpy.mockReturnValue(
44+
new URL('https://api.github.com/'),
45+
);
46+
47+
const octokit = await createOctokitClient(
48+
mockGitHubCloudAccount,
49+
'graphql',
50+
);
51+
52+
expect(getGitHubAPIBaseUrlSpy).toHaveBeenCalledWith(
53+
'github.com',
54+
'graphql',
55+
);
56+
expect(octokit).toBeDefined();
57+
expect(mockDecryptValue).toHaveBeenCalledWith(
58+
mockGitHubCloudAccount.token,
59+
);
3860
});
3961

40-
it('should create octokit client for GitHub Enterprise Server', async () => {
62+
it('should create octokit rest client for GitHub Enterprise Server', async () => {
4163
const getGitHubAPIBaseUrlSpy = jest.spyOn(utils, 'getGitHubAPIBaseUrl');
4264
getGitHubAPIBaseUrlSpy.mockReturnValue(
4365
new URL('https://github.gitify.io/api/v3/'),
4466
);
4567

4668
const octokit = await createOctokitClient(
4769
mockGitHubEnterpriseServerAccount,
70+
'rest',
71+
);
72+
73+
expect(getGitHubAPIBaseUrlSpy).toHaveBeenCalledWith(
74+
'github.gitify.io',
75+
'rest',
76+
);
77+
expect(octokit).toBeDefined();
78+
expect(mockDecryptValue).toHaveBeenCalledWith(
79+
mockGitHubEnterpriseServerAccount.token,
80+
);
81+
});
82+
83+
it('should create octokit graphql client for GitHub Enterprise Server', async () => {
84+
const getGitHubAPIBaseUrlSpy = jest.spyOn(utils, 'getGitHubAPIBaseUrl');
85+
getGitHubAPIBaseUrlSpy.mockReturnValue(
86+
new URL('https://github.gitify.io/api/graphql/'),
87+
);
88+
89+
const octokit = await createOctokitClient(
90+
mockGitHubEnterpriseServerAccount,
91+
'graphql',
4892
);
4993

50-
expect(getGitHubAPIBaseUrlSpy).toHaveBeenCalledWith('github.gitify.io');
94+
expect(getGitHubAPIBaseUrlSpy).toHaveBeenCalledWith(
95+
'github.gitify.io',
96+
'graphql',
97+
);
5198
expect(octokit).toBeDefined();
52-
expect(mockDecryptValue).toHaveBeenCalledWith(mockToken);
99+
expect(mockDecryptValue).toHaveBeenCalledWith(
100+
mockGitHubEnterpriseServerAccount.token,
101+
);
53102
});
54103

55-
it('should cache and reuse octokit clients for the same account', async () => {
104+
it('should cache and reuse octokit clients for the same account and api type', async () => {
56105
const getGitHubAPIBaseUrlSpy = jest.spyOn(utils, 'getGitHubAPIBaseUrl');
57106
getGitHubAPIBaseUrlSpy.mockReturnValue(
58107
new URL('https://api.github.com/'),
59108
);
60109

61-
const octokit1 = await createOctokitClient(mockGitHubCloudAccount);
110+
const octokit1 = await createOctokitClient(
111+
mockGitHubCloudAccount,
112+
'rest',
113+
);
62114

63-
const octokit2 = await createOctokitClient(mockGitHubCloudAccount);
115+
const octokit2 = await createOctokitClient(
116+
mockGitHubCloudAccount,
117+
'rest',
118+
);
64119

65120
// Should return the same instance
66121
expect(octokit1).toBe(octokit2);
@@ -72,15 +127,41 @@ describe('renderer/utils/api/octokit.ts', () => {
72127
expect(getGitHubAPIBaseUrlSpy).toHaveBeenCalledTimes(1);
73128
});
74129

75-
it('should create different clients for different accounts', async () => {
130+
it('should create different clients for different accounts with same api type', async () => {
131+
const getGitHubAPIBaseUrlSpy = jest.spyOn(utils, 'getGitHubAPIBaseUrl');
132+
getGitHubAPIBaseUrlSpy.mockReturnValue(
133+
new URL('https://api.github.com/'),
134+
);
135+
136+
const octokit1 = await createOctokitClient(mockGitHubAppAccount, 'rest');
137+
138+
const octokit2 = await createOctokitClient(
139+
mockGitHubCloudAccount,
140+
'rest',
141+
);
142+
143+
// Should be different instances for different tokens
144+
expect(octokit1).not.toBe(octokit2);
145+
146+
// Should decrypt both tokens
147+
expect(mockDecryptValue).toHaveBeenCalledTimes(2);
148+
});
149+
150+
it('should create different clients for same accounts with different api type', async () => {
76151
const getGitHubAPIBaseUrlSpy = jest.spyOn(utils, 'getGitHubAPIBaseUrl');
77152
getGitHubAPIBaseUrlSpy.mockReturnValue(
78153
new URL('https://api.github.com/'),
79154
);
80155

81-
const octokit1 = await createOctokitClient(mockGitHubAppAccount);
156+
const octokit1 = await createOctokitClient(
157+
mockGitHubCloudAccount,
158+
'rest',
159+
);
82160

83-
const octokit2 = await createOctokitClient(mockGitHubCloudAccount);
161+
const octokit2 = await createOctokitClient(
162+
mockGitHubCloudAccount,
163+
'graphql',
164+
);
84165

85166
// Should be different instances for different tokens
86167
expect(octokit1).not.toBe(octokit2);

‎src/renderer/utils/api/octokit.ts‎

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { restEndpointMethods } from '@octokit/plugin-rest-endpoint-methods';
55
import { APPLICATION } from '../../../shared/constants';
66

77
import type { Account } from '../../types';
8+
import type { APIClientType } from './types';
89

910
import { getAccountUUID } from '../auth/utils';
1011
import { decryptValue, getAppVersion } from '../comms';
@@ -14,7 +15,7 @@ import { getGitHubAPIBaseUrl } from './utils';
1415
const OctokitWithPlugins = Octokit.plugin(paginateRest, restEndpointMethods);
1516
export type OctokitClient = InstanceType<typeof OctokitWithPlugins>;
1617

17-
// Cache Octokit clients per account UUID
18+
// Cache Octokit clients per account UUID + type (rest|graphql)
1819
const octokitClientCache = new Map<string, OctokitClient>();
1920

2021
const version = getAppVersion();
@@ -36,11 +37,13 @@ export function clearOctokitClientCache(): void {
3637
*/
3738
export async function createOctokitClient(
3839
account: Account,
40+
type: APIClientType,
3941
): Promise<OctokitClient> {
4042
const accountUUID = getAccountUUID(account);
43+
const cacheKey = `${accountUUID}:${type}`;
4144

4245
// Return cached client if it exists
43-
const cachedClient = octokitClientCache.get(accountUUID);
46+
const cachedClient = octokitClientCache.get(cacheKey);
4447
if (cachedClient) {
4548
return cachedClient;
4649
}
@@ -49,18 +52,28 @@ export async function createOctokitClient(
4952
const decryptedToken = await decryptValue(account.token);
5053

5154
// Get the base API URL for the hostname
52-
const baseUrl = getGitHubAPIBaseUrl(account.hostname)
55+
const baseUrl = getGitHubAPIBaseUrl(account.hostname, type)
5356
.toString()
5457
.replace(/\/$/, '');
5558

59+
const userAgent = getUserAgent();
60+
5661
const client = new OctokitWithPlugins({
5762
auth: decryptedToken,
5863
baseUrl,
59-
userAgent: `${APPLICATION.NAME}/${version}`,
64+
userAgent: userAgent,
6065
});
6166

62-
// Cache the client
63-
octokitClientCache.set(accountUUID, client);
67+
// Cache the client keyed by account UUID + type
68+
octokitClientCache.set(cacheKey, client);
6469

6570
return client;
6671
}
72+
73+
/**
74+
* Format Gitify User Agent
75+
* @returns User Agent to be set for API requests
76+
*/
77+
export function getUserAgent() {
78+
return `${APPLICATION.NAME}/${version}`;
79+
}

‎src/renderer/utils/api/request.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ export async function performGraphQLRequest<TResult, TVariables>(
2121
// FIXME graphql base url for enterprise accounts
2222
// const url = getGitHubGraphQLUrl(account.hostname);
2323

24-
const octokit = await createOctokitClient(account);
24+
const octokit = await createOctokitClient(account, 'graphql');
2525

2626
try {
2727
const response = await octokit.graphql<TResult>(
@@ -73,7 +73,7 @@ export async function performGraphQLRequestString<TResult>(
7373
// FIXME graphql base url for enterprise accounts
7474
// const url = getGitHubGraphQLUrl(account.hostname);
7575

76-
const octokit = await createOctokitClient(account);
76+
const octokit = await createOctokitClient(account, 'graphql');
7777

7878
try {
7979
const response = await octokit.graphql<TResult>(query, variables || {});

‎src/renderer/utils/api/types.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
import type { components } from '@octokit/openapi-types';
22
import type { Endpoints } from '@octokit/types';
33

4+
export type APIClientType = 'rest' | 'graphql';
5+
46
export type ListNotificationsForAuthenticatedUserResponse =
57
Endpoints['GET /notifications']['response']['data'];
68

‎src/renderer/utils/api/utils.test.ts‎

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,6 @@
11
import type { Hostname } from '../../types';
22

3-
import {
4-
getGitHubAPIBaseUrl,
5-
getGitHubAuthBaseUrl,
6-
getGitHubGraphQLUrl,
7-
} from './utils';
3+
import { getGitHubAPIBaseUrl, getGitHubAuthBaseUrl } from './utils';
84

95
describe('renderer/utils/api/utils.ts', () => {
106
describe('getGitHubAuthBaseUrl', () => {
@@ -20,26 +16,30 @@ describe('renderer/utils/api/utils.ts', () => {
2016
});
2117

2218
describe('getGitHubAPIBaseUrl', () => {
23-
it('should generate a GitHub API url - non enterprise', () => {
24-
const result = getGitHubAPIBaseUrl('github.com' as Hostname);
19+
it('should generate a GitHub REST API url - non enterprise', () => {
20+
const result = getGitHubAPIBaseUrl('github.com' as Hostname, 'rest');
2521
expect(result.toString()).toBe('https://api.github.com/');
2622
});
2723

28-
it('should generate a GitHub API url - enterprise', () => {
29-
const result = getGitHubAPIBaseUrl('github.gitify.io' as Hostname);
24+
it('should generate a GitHub REST API url - enterprise', () => {
25+
const result = getGitHubAPIBaseUrl(
26+
'github.gitify.io' as Hostname,
27+
'rest',
28+
);
3029
expect(result.toString()).toBe('https://github.gitify.io/api/v3/');
3130
});
3231
});
3332

34-
describe('getGitHubGraphQLUrl', () => {
35-
it('should generate a GitHub GraphQL url - non enterprise', () => {
36-
const result = getGitHubGraphQLUrl('github.com' as Hostname);
37-
expect(result.toString()).toBe('https://api.github.com/graphql');
38-
});
33+
it('should generate a GitHub GraphQL url - non enterprise', () => {
34+
const result = getGitHubAPIBaseUrl('github.com' as Hostname, 'graphql');
35+
expect(result.toString()).toBe('https://api.github.com/graphql');
36+
});
3937

40-
it('should generate a GitHub GraphQL url - enterprise', () => {
41-
const result = getGitHubGraphQLUrl('github.gitify.io' as Hostname);
42-
expect(result.toString()).toBe('https://github.gitify.io/api/graphql');
43-
});
38+
it('should generate a GitHub GraphQL url - enterprise', () => {
39+
const result = getGitHubAPIBaseUrl(
40+
'github.gitify.io' as Hostname,
41+
'graphql',
42+
);
43+
expect(result.toString()).toBe('https://github.gitify.io/api/graphql');
4444
});
4545
});

0 commit comments

Comments
 (0)