-
Notifications
You must be signed in to change notification settings - Fork 662
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
Web API Type Safety #1673
Web API Type Safety #1673
Changes from 30 commits
ec34ce6
79aaf6e
62ffc7f
3cb6c62
16464e5
b4837a4
e2d2faa
bc25c1f
41a0d79
931c06f
2e9f906
3a4a7a2
7e7b16c
4e0a739
a3cfd8b
124e5f6
2bf4dc9
b9c63c8
daed47d
202a321
4fca944
d64c44d
fd17314
6813605
6d26fda
a526bec
6afd88a
0a2692d
75ef263
9dea3ab
6ce56f3
f172551
8766ead
5864f31
822496f
9f8df3c
5345456
fe414b3
6860612
206078c
64950e5
82b82f6
539ee1d
e6adeb0
4a97841
98f22c2
ac6ca14
fa547db
08b4d0f
b7749a5
896edd4
7a0724f
a52a5ce
753d15d
53c0034
5a5cdb1
11c69d2
0e24e84
352f985
336c62d
3c8d14a
9446539
f35fb45
59026ca
3eaf51b
2996db8
e9ac27a
7065af8
7efacf7
d2a7882
bb042dc
13f0b0e
a8c8dc5
524493d
3f5a661
0bdd2af
75a1e95
1db4e03
4bea006
c3af002
5a1a354
cd81272
2ae4033
a50ae8e
d887ef1
4d83293
2ffa1bd
587b71c
658cf45
a29e77a
6cc5f84
4956cc8
efec9ea
71fd621
38b1a4e
40f706c
c3aaac4
73e1a9e
64529a0
6323f6e
f453f2e
a3db517
80c3d5e
d548bff
1d94a70
3687c89
94c7c57
08d1413
7d03359
38dfcde
692792f
456c49b
2f1f9da
22efb5a
0bec7f6
0f59b96
fcebf0e
b6f1a88
d0526bf
260e242
a193b6e
f01e0e6
920c3fb
6e2d515
f340482
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -64,10 +64,6 @@ export enum WebClientEvent { | |
RATE_LIMITED = 'rate_limited', | ||
} | ||
|
||
export interface WebAPICallOptions { | ||
[argument: string]: unknown; | ||
} | ||
|
||
export interface WebAPICallResult { | ||
ok: boolean; | ||
error?: string; | ||
|
@@ -82,7 +78,6 @@ export interface WebAPICallResult { | |
// `chat.postMessage` returns an array of error messages (e.g., "messages": ["[ERROR] invalid_keys"]) | ||
messages?: string[]; | ||
}; | ||
[key: string]: unknown; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similarly, this change now prevents responses from being, basically, any shape. |
||
} | ||
|
||
// NOTE: should there be an async predicate? | ||
|
@@ -224,7 +219,7 @@ export class WebClient extends Methods { | |
* @param method - the Web API method to call {@link https://api.slack.com/methods} | ||
* @param options - options | ||
*/ | ||
public async apiCall(method: string, options: WebAPICallOptions = {}): Promise<WebAPICallResult> { | ||
public async apiCall(method: string, options: Record<string, unknown> = {}): Promise<WebAPICallResult> { | ||
this.logger.debug(`apiCall('${method}') start`); | ||
|
||
warnDeprecations(method, this.logger); | ||
|
@@ -304,21 +299,21 @@ export class WebClient extends Methods { | |
* @param shouldStop - a predicate that is called with each page, and should return true when pagination can end. | ||
* @param reduce - a callback that can be used to accumulate a value that the return promise is resolved to | ||
*/ | ||
public paginate(method: string, options?: WebAPICallOptions): AsyncIterable<WebAPICallResult>; | ||
public paginate(method: string, options?: Record<string, unknown>): AsyncIterable<WebAPICallResult>; | ||
public paginate( | ||
method: string, | ||
options: WebAPICallOptions, | ||
options: Record<string, unknown>, | ||
shouldStop: PaginatePredicate, | ||
): Promise<void>; | ||
public paginate<R extends PageReducer, A extends PageAccumulator<R>>( | ||
method: string, | ||
options: WebAPICallOptions, | ||
options: Record<string, unknown>, | ||
shouldStop: PaginatePredicate, | ||
reduce?: PageReducer<A>, | ||
): Promise<A>; | ||
public paginate<R extends PageReducer, A extends PageAccumulator<R>>( | ||
method: string, | ||
options?: WebAPICallOptions, | ||
options?: Record<string, unknown>, | ||
shouldStop?: PaginatePredicate, | ||
reduce?: PageReducer<A>, | ||
): (Promise<A> | AsyncIterable<WebAPICallResult>) { | ||
|
@@ -394,22 +389,22 @@ export class WebClient extends Methods { | |
})(); | ||
} | ||
|
||
/* eslint-disable no-trailing-spaces */ | ||
/** | ||
* This wrapper method provides an easy way to upload files using the following endpoints: | ||
* | ||
* | ||
* **#1**: For each file submitted with this method, submit filenames | ||
* and file metadata to {@link https://api.slack.com/methods/files.getUploadURLExternal files.getUploadURLExternal} to request a URL to | ||
* which to send the file data to and an id for the file | ||
* | ||
* | ||
* **#2**: for each returned file `upload_url`, upload corresponding file to | ||
* URLs returned from step 1 (e.g. https://files.slack.com/upload/v1/...\") | ||
* | ||
* | ||
* **#3**: Complete uploads {@link https://api.slack.com/methods/files.completeUploadExternal files.completeUploadExternal} | ||
* | ||
* @param options | ||
*/ | ||
public async filesUploadV2(options: FilesUploadV2Arguments): Promise<WebAPICallResult> { | ||
public async filesUploadV2(options: FilesUploadV2Arguments): Promise< | ||
WebAPICallResult & { files: FilesCompleteUploadExternalResponse[] } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Small tweak to make this API a bit more type safe with its response. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good catch! |
||
> { | ||
this.logger.debug('files.uploadV2() start'); | ||
// 1 | ||
const fileUploads = await this.getAllFileUploads(options); | ||
|
@@ -425,7 +420,7 @@ export class WebClient extends Methods { | |
|
||
// 3 | ||
const completion = await this.completeFileUploads(fileUploads); | ||
|
||
return { ok: true, files: completion }; | ||
} | ||
|
||
|
@@ -456,7 +451,7 @@ export class WebClient extends Methods { | |
* @returns | ||
*/ | ||
private async completeFileUploads(fileUploads: FileUploadV2Job[]): | ||
Promise<Array<FilesCompleteUploadExternalResponse>> { | ||
Promise<Array<FilesCompleteUploadExternalResponse>> { | ||
const toComplete: FilesCompleteUploadExternalArguments[] = Object.values(getAllFileUploadsToComplete(fileUploads)); | ||
return Promise.all( | ||
toComplete.map((job: FilesCompleteUploadExternalArguments) => this.files.completeUploadExternal(job)), | ||
|
@@ -485,10 +480,10 @@ export class WebClient extends Methods { | |
}, headers); | ||
if (uploadRes.status !== 200) { | ||
return Promise.reject(Error(`Failed to upload file (id:${file_id}, filename: ${filename})`)); | ||
} | ||
} | ||
const returnData = { ok: true, body: uploadRes.data } as WebAPICallResult; | ||
return Promise.resolve(returnData); | ||
} | ||
} | ||
return Promise.reject(Error(`No upload url found for file (id: ${file_id}, filename: ${filename}`)); | ||
})); | ||
} | ||
|
@@ -504,7 +499,7 @@ export class WebClient extends Methods { | |
if (options.file || options.content) { | ||
fileUploads.push(await getFileUploadJob(options, this.logger)); | ||
} | ||
|
||
// add multiple files data when file_uploads is supplied | ||
if (options.file_uploads) { | ||
fileUploads = fileUploads.concat(await getMultipleFileUploadJobs(options, this.logger)); | ||
|
@@ -594,8 +589,8 @@ export class WebClient extends Methods { | |
* @param options - arguments for the Web API method | ||
* @param headers - a mutable object representing the HTTP headers for the outgoing request | ||
*/ | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
private serializeApiCallOptions(options: WebAPICallOptions, headers?: any): string | Readable { | ||
private serializeApiCallOptions(options: Record<string, unknown>, headers?: Record<string, string>): string | | ||
Readable { | ||
// The following operation both flattens complex objects into a JSON-encoded strings and searches the values for | ||
// binary content | ||
let containsBinaryData: boolean = false; | ||
|
@@ -648,18 +643,20 @@ export class WebClient extends Methods { | |
}, | ||
new FormData(), | ||
); | ||
// Copying FormData-generated headers into headers param | ||
// not reassigning to headers param since it is passed by reference and behaves as an inout param | ||
Object.entries(form.getHeaders()).forEach(([header, value]) => { | ||
// eslint-disable-next-line no-param-reassign | ||
headers[header] = value; | ||
}); | ||
if (headers) { | ||
// Copying FormData-generated headers into headers param | ||
// not reassigning to headers param since it is passed by reference and behaves as an inout param | ||
Object.entries(form.getHeaders()).forEach(([header, value]) => { | ||
// eslint-disable-next-line no-param-reassign | ||
headers[header] = value; | ||
}); | ||
} | ||
return form; | ||
} | ||
|
||
// Otherwise, a simple key-value object is returned | ||
// eslint-disable-next-line no-param-reassign | ||
headers['Content-Type'] = 'application/x-www-form-urlencoded'; | ||
if (headers) headers['Content-Type'] = 'application/x-www-form-urlencoded'; | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
const initialValue: { [key: string]: any; } = {}; | ||
return qsStringify(flattened.reduce( | ||
|
@@ -823,16 +820,16 @@ function warnDeprecations(method: string, logger: Logger): void { | |
* @param logger instance of we clients logger | ||
* @param options arguments for the Web API method | ||
*/ | ||
function warnIfFallbackIsMissing(method: string, logger: Logger, options?: WebAPICallOptions): void { | ||
function warnIfFallbackIsMissing(method: string, logger: Logger, options?: Record<string, unknown>): void { | ||
const targetMethods = ['chat.postEphemeral', 'chat.postMessage', 'chat.scheduleMessage']; | ||
const isTargetMethod = targetMethods.includes(method); | ||
|
||
const hasAttachments = (args: WebAPICallOptions) => Array.isArray(args.attachments) && args.attachments.length; | ||
const hasAttachments = (args: Record<string, unknown>) => Array.isArray(args.attachments) && args.attachments.length; | ||
|
||
const missingAttachmentFallbackDetected = (args: WebAPICallOptions) => Array.isArray(args.attachments) && | ||
const missingAttachmentFallbackDetected = (args: Record<string, unknown>) => Array.isArray(args.attachments) && | ||
args.attachments.some((attachment) => !attachment.fallback || attachment.fallback.trim() === ''); | ||
|
||
const isEmptyText = (args: WebAPICallOptions) => args.text === undefined || args.text === null || args.text === ''; | ||
const isEmptyText = (args: Record<string, unknown>) => args.text === undefined || args.text === null || args.text === ''; | ||
|
||
const buildMissingTextWarning = () => `The top-level \`text\` argument is missing in the request payload for a ${method} call - ` + | ||
'It\'s a best practice to always provide a `text` argument when posting a message. ' + | ||
|
@@ -860,23 +857,23 @@ function warnIfFallbackIsMissing(method: string, logger: Logger, options?: WebAP | |
* @param logger instance of web clients logger | ||
* @param options arguments for the Web API method | ||
*/ | ||
function warnIfThreadTsIsNotString(method: string, logger: Logger, options?: WebAPICallOptions): void { | ||
function warnIfThreadTsIsNotString(method: string, logger: Logger, options?: Record<string, unknown>): void { | ||
const targetMethods = ['chat.postEphemeral', 'chat.postMessage', 'chat.scheduleMessage', 'files.upload']; | ||
const isTargetMethod = targetMethods.includes(method); | ||
|
||
if (isTargetMethod && options?.thread_ts !== undefined && typeof options?.thread_ts !== 'string') { | ||
logger.warn(buildThreadTsWarningMessage(method)); | ||
} | ||
} | ||
|
||
export function buildThreadTsWarningMessage(method: string): string { | ||
export function buildThreadTsWarningMessage(method: string): string { | ||
return `The given thread_ts value in the request payload for a ${method} call is a float value. We highly recommend using a string value instead.`; | ||
} | ||
|
||
/** | ||
* Takes an object and redacts specific items | ||
* @param body | ||
* @returns | ||
* @param body | ||
* @returns | ||
*/ | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
function redact(body: any): any { | ||
|
@@ -886,7 +883,7 @@ function redact(body: any): any { | |
if (value === undefined || value === null) { | ||
return []; | ||
} | ||
|
||
let serializedValue = value; | ||
|
||
// redact possible tokens | ||
|
@@ -903,7 +900,7 @@ function redact(body: any): any { | |
return [key, serializedValue]; | ||
}); | ||
|
||
// return as object | ||
// return as object | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
const initialValue: { [key: string]: any; } = {}; | ||
return flattened.reduce( | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing this interface makes the arguments to each API method now 'type safe' (more or less) in that developers can't provide whatever arguments they want anymore (at least not without overriding/ignoring TypeScript).
We can more native-TypeScript-y instead use
Record<string, unknown>
to model any shapeless object in TypeScript (which the rest of this PR does where applicable).