Impl uploadv2 #90

taiga533 · 2023-12-15T12:55:44Z

Summary

Implement the fileUploadV2 method, which allows for the uploading of multiple files.
Implement the files.getUploadURLExternal method.
Implement the files.completeUploadExternal method.

testing

If you want to test with the actual Slack API, you could run a script like this.

// run your local machine
import { SlackAPI } from "./../deno-slack-api/src/mod.ts";

const token =
  "your-token-here";

// initialize Slack API client
const slack = SlackAPI(token);

console.log(
  await slack.fileUploadV2({
    file_uploads: [
      {
        file: "sample",
        length: "6",
        filename: "sample.txt",
      },
    ],
  }),
);

Special notes

Based on this issue, I implemented it.

Requirements

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.
I've ran deno task test after making the changes.

Generate files.getUploadURLExternal and files.completeUploadExternal.

and add new tests to api_test.ts.

salesforce-cla · 2023-12-15T12:55:49Z

Thanks for the contribution! Unfortunately we can't verify the commit author(s): kawasaki.taiga <k***@r***.n***.jp>. One possible solution is to add that email to your GitHub account. Alternatively you can change your commits to another email and force push the change. After getting your commits associated with your GitHub account, sign the Salesforce Inc. Contributor License Agreement and this Pull Request will be revalidated.

WilliamBergamin

This is awesome 💯 @taiga533 thank you for the contribution 🥇

I left a few initial comments, there may be a few things to iron out since custom methods seem to be uncharted territory for this library

WilliamBergamin · 2023-12-15T18:42:01Z

src/typed-method-types/files.ts

@@ -0,0 +1,32 @@
+import { BaseResponse } from "../types.ts";
+
+export type FileUploadV2 = {


What do you think about potentially implementing the type for FileUpload similar to the node sdk and then building this type from it?

That's a good idea. By having separate types for FileUpload and FileUploadV2, the FileUpload type will be useful when trying to implement the old files.upload. Thanks for the advice.
I implemented.

WilliamBergamin · 2023-12-15T18:46:19Z

src/typed-method-types/files.ts

+};
+
+export type FileUploadV2Args = {
+  file_uploads: FileUploadV2[];


@filmaj seems like the node-sdk has a deprecated additional field request_file_info since it does not appear here would we want to do something like FileUploadV2Args = FileUploadV2[]?

FileUploadV2Args might indeed be better as a FileUploadV2[] type. Is it okay to modify it that way? 😄

While it may seem awkward to nest it under an additional property name, I believe having consistency across all SDKs is more important than a sense of property cleanliness. My vote would be to keep the structure as-is, to allow for easier conceptual movement from one language / bolt framework to another.

WilliamBergamin · 2023-12-15T19:02:09Z

src/typed-method-types/files.ts

+  /** @description Name of the file being uploaded. */
+  filename: string;
+  /** @description Filetype of the file being uploaded. */
+  file: Blob | ReadableStream<Uint8Array> | string | ArrayBuffer;


I think the content field is missing from this type 🤔 (node sdk content field) was this omitted for a specific reason?

I didn't implement the content field because it's not usable for file upload v2 (when uploading via getExternalUploadURL). In node-slack-sdk, whether you use the content or file field, the data is processed like the file field and an HTTP request is made. reference

I learned from @filmaj that the content field is an important element! I was mistaken in my previous comment, so I will implement the content field. 🙇‍♂️

#90 (comment)

codecov · 2023-12-15T19:07:38Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7476afc) 100.00% compared to head (b735fec) 100.00%.

❗ Current head b735fec differs from pull request most recent head be9ee11. Consider uploading reports for the commit be9ee11 to get more accurate results

Additional details and impacted files

@@            Coverage Diff            @@
##              main       #90   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           36        36           
  Lines         1091      1155   +64     
  Branches        13        22    +9     
=========================================
+ Hits          1091      1155   +64

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

taiga533 · 2023-12-18T14:17:36Z

@WilliamBergamin
Thank you for your review! 😄 ⭐

My implementation, while referencing the node-slack-api, avoids implementing fields that will not be supported in the future and includes fields that will become mandatory as mandatory fields.

Since node-slack-api has been around for a long time, there's a benefit to aligning the fields used in fileUploadV2 with those used in files.upload. However, in deno-slack-api, files.upload is currently not usable, so I thought there was no need to implement fields like content or filetype.

Modify the definition of the FileUploadV2 type to extend the FileUpload type. And modify the FileUpload type so that it does not directly reference ReadableStream, ensuring npm build succeeds.

filmaj

Hey thanks for taking a stab at this! This is really great!

I have left some changes that I think we require in order to keep parity with the same method from the other SDKs.

Another request: would it be possible to somehow integrate this method so that it shows up under the client.files.* property? I understand that everything under files is a generated method, but I wanted to ask everyone involved in this discussion, @WilliamBergamin included, if perhaps we can come up with a way to:

Place the method under the files property, and
Rename the method to uploadV2

In this way, it would exactly match the "New Way" we describe the file uploading process in our node SDK docs.

filmaj · 2023-12-18T18:59:54Z

src/base-client.ts

+  ) {
+    const response = await fetch(uploadUrl, {
+      headers: {
+        "Content-Type": typeof file === "string"


I don't think this works the same way as the node SDK. If file is a string in the node SDK, then the assumption is that the file argument is a file path to the file data. This is why the content parameter exists on the node SDK: so that users who do want to provide string-based file contents can do so.

Can we make this work the same way as the node SDK?

Also for reference: the code for how the python SDK handles these parameters.

I had overlooked how the node SDK behaves when a string is passed to the file field 😮. It's definitely not good for the behavior to differ between SDKs. I will make corrections so that the handling of the content field and file field behaves the same as in other SDKs.

filmaj · 2023-12-18T19:16:04Z

src/base-client.ts

+    file: FileUploadV2["file"],
+  ) {
+    const response = await fetch(uploadUrl, {
+      headers: {


When actually POSTing the file contents to the URL provided by getUploadURLExternal, we post the data as binary (raw bytes), or multipart-form-encoded (see the docs on this here).

As a result, I don't think we should be providing headers like Content-Type. The file-upload-external edge URL that we POST file data to will infer the content type based on the data uploaded.

In the node SDK, the headers are empty other than the token.
Similarly, in the python SDK, same thing.

I think the trick in implementing this API is to ensure we encode the POST data in the appropriate format, based on what is provided by the user. Assuming we support the content parameter, like the other SDKs do, then the content parameter, which is a string, should be encoded as UTF-8 bytes. Otherwise, if file is a string, we should interpret that string as a file path, and read the raw bytes directly from that path. Finally, file may also be a buffer-like object. In this situation, we should provide the raw buffer directly to fetch as its data payload.

OK. Indeed, delegating the setting of Content-Type to the fetch function is appropriate. Thank you for the detailed explanation 😊. Based on this, I will make the following modifications to the source code:

When uploading a file to the URL obtained from getUploadURLExternal, do not set the Content-Type.

Implement a content field, read its value as UTF-8, and pass it to the request body of the fetch function.

When a string is passed to the file field, interpret it as a file path and read the file using Deno.readFileSync.

racTaiga533 added 2 commits December 15, 2023 17:20

Generate files.uploadV2 methods.

5ab4181

Generate files.getUploadURLExternal and files.completeUploadExternal.

Add fileUploadV2 method to BaseSlackAPIClient

b735fec

and add new tests to api_test.ts.

taiga533 requested a review from a team as a code owner December 15, 2023 12:55

salesforce-cla bot added the cla:missing label Dec 15, 2023

salesforce-cla bot added cla:signed and removed cla:missing labels Dec 15, 2023

WilliamBergamin requested changes Dec 15, 2023

View reviewed changes

Refactor file upload types in files.ts.

be9ee11

Modify the definition of the FileUploadV2 type to extend the FileUpload type. And modify the FileUpload type so that it does not directly reference ReadableStream, ensuring npm build succeeds.

taiga533 requested a review from WilliamBergamin December 18, 2023 14:27

filmaj requested changes Dec 18, 2023

View reviewed changes

seratch mentioned this pull request Mar 5, 2024

[QUERY] How to use files.upload to upload a binary file #94

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Impl uploadv2 #90

Impl uploadv2 #90

taiga533 commented Dec 15, 2023

salesforce-cla bot commented Dec 15, 2023

WilliamBergamin left a comment

WilliamBergamin Dec 15, 2023

taiga533 Dec 18, 2023

WilliamBergamin Dec 15, 2023

taiga533 Dec 18, 2023

filmaj Dec 18, 2023

WilliamBergamin Dec 15, 2023

taiga533 Dec 18, 2023

taiga533 Dec 20, 2023

codecov bot commented Dec 15, 2023 •

edited

taiga533 commented Dec 18, 2023

filmaj left a comment

filmaj Dec 18, 2023

taiga533 Dec 20, 2023

filmaj Dec 18, 2023

taiga533 Dec 20, 2023

		@@ -0,0 +1,32 @@
		import { BaseResponse } from "../types.ts";

		export type FileUploadV2 = {

Impl uploadv2 #90

Are you sure you want to change the base?

Impl uploadv2 #90

Conversation

taiga533 commented Dec 15, 2023

Summary

testing

Special notes

Requirements

salesforce-cla bot commented Dec 15, 2023

WilliamBergamin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 15, 2023 • edited

Codecov Report

taiga533 commented Dec 18, 2023

filmaj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 15, 2023 •

edited