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

[Test PR] SDK Multipart support #93

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

vermakhushboo
Copy link
Member

What does this PR do?

Example usage:

const execution = await functions.createExecution(
        '1', // functionId
        null, // body (optional)
);
const output = execution.responseBody.toBinary();

Test Plan

(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work.)

Related PRs and Issues

(If this PR is related to any other PR or resolves any issue or related to any issue link all related PR and issues here.)

Have you read the Contributing Guidelines on issues?

(Write your answer here.)

package.json Show resolved Hide resolved
src/NewPayload.ts Outdated Show resolved Hide resolved
src/NewPayload.ts Outdated Show resolved Hide resolved
src/NewPayload.ts Outdated Show resolved Hide resolved
src/NewPayload.ts Outdated Show resolved Hide resolved
src/NewPayload.ts Outdated Show resolved Hide resolved
src/NewPayload.ts Outdated Show resolved Hide resolved
return Uint8Array.from(this.data);
}

public getData(): any {
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets avoid any

Copy link
Member

Choose a reason for hiding this comment

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

If you make the Payload class parameterised like Payload
Then you would be able to return proper types.

src/client.ts Outdated Show resolved Hide resolved
src/client.ts Outdated Show resolved Hide resolved
@@ -33,3 +33,5 @@ export { ImageGravity } from './enums/image-gravity';
export { ImageFormat } from './enums/image-format';
export { PasswordHash } from './enums/password-hash';
export { MessagingProviderType } from './enums/messaging-provider-type';
export { NewPayload } from './NewPayload';
export { InputFile } from './inputFile';
Copy link
Contributor

Choose a reason for hiding this comment

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

We should remove all mention of InputFile. It should no longer exist.

With that, we also need to update existing endpoints that used to need it. they all should now use payload class

Copy link
Contributor

Choose a reason for hiding this comment

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

I dont see type change.. like ExecutionModel.. How do they know about this new Payload class being used for responseBody?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have added Payload response model in appwrite. Do we need to make any other change in SDK to use it?

Copy link
Contributor

Choose a reason for hiding this comment

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

src/client.ts Outdated
}, {});
data = {
...partsObject,
responseBody: new NewPayload(partsObject.responseBody)
Copy link
Contributor

Choose a reason for hiding this comment

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

ResponseBody can be empty. in which case it would probably be.. empty string? in which case, it might not fit the buffer type? Just thinking out lit. its something to test - to make async execution (that doesn't have body) and see if all works fine

if (typeof functionId === 'undefined') {
throw new AppwriteException('Missing required parameter: "functionId"');
}
const apiPath = '/functions/{functionId}/executions'.replace('{functionId}', functionId);
const payload: Payload = {};
if (typeof body !== 'undefined') {
payload['body'] = body;
payload['body'] = body ? body.getData : body;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not getData(), with ()?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I dont understand the check. It doesn't make sense, right?

tsconfig.json Outdated Show resolved Hide resolved
// convert binary data (Buffer) to file
async toFile(fileName: string): Promise<File> {
const blob = new Blob([this.data]);
return new File([blob], fileName);
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't File class used in HTML File input? Or it is Node class? I am not familiar with it

}

// convert binary data (Buffer) to file
async toFile(fileName: string): Promise<File> {
Copy link
Contributor

Choose a reason for hiding this comment

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

toPath might also make sense. It would use fs.writeFileSync to write it to disk, similarly how fromPath reads it using fs

}

// converts text to binary data (Buffer)
static async fromPlainText(text: string): Promise<NewPayload> {
Copy link
Member

Choose a reason for hiding this comment

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

return type is not a promise and remove async from function

}

// converts binary data (Buffer) to JSON
async toJson(): Promise<any> {
Copy link
Member

Choose a reason for hiding this comment

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

again no async and return type shouldn't be a promise..

Also instead of any, is there any other type we can return that more closely resembles a JSON object? any means it can be string, number etc.

src/client.ts Outdated
return acc;
}, {});
data = {
...partsObject,
Copy link
Contributor

@Meldiron Meldiron Aug 14, 2024

Choose a reason for hiding this comment

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

I think everything in multipart response is string. Try createExecution, and print it. See what is type of responseStatusCode, for example. It should be integer and I am worried it will be string. We need proper support for all types, as well as JSON (array/object).

@@ -0,0 +1,47 @@
import { realpathSync, readFileSync } from "fs";
Copy link
Member

Choose a reason for hiding this comment

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

If we import fs this can't be used from Edge runtimes, such as Vercel/Cloudflare or Deno

public static fromPath(path: string): Buffer {
const realPath = realpathSync(path);
const contents = readFileSync(realPath);
return Buffer.from(contents);
Copy link
Contributor

Choose a reason for hiding this comment

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

Manually QA fromFile and fromPath. Upload a picture to Storage service. Then see the picture in Appwrite Console. Does it know it's PNG? Can you preview it? If not, we need to fix it.

for (const part of parts) {
if (part.name) {
if (part.name === "responseBody") {
partsObject[part.name] = part.data;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
partsObject[part.name] = part.data;
partsObject[part.name] = new Payload(part.data)

I think

if (part.name === "responseBody") {
partsObject[part.name] = part.data;
} else {
partsObject[part.name] = part.data.toString();
Copy link
Contributor

Choose a reason for hiding this comment

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

What if it's array or integer or boolean or object. Address when moving to sdk-generator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants