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: Pass parameters as an object #728

Open
2 tasks done
dokgu opened this issue Oct 20, 2023 · 10 comments
Open
2 tasks done

🚀 Feature: Pass parameters as an object #728

dokgu opened this issue Oct 20, 2023 · 10 comments

Comments

@dokgu
Copy link

dokgu commented Oct 20, 2023

🔖 Feature description

Pass the parameters as an object - something like:

type CreateMembershipProps = {
  teamId: string;
  roles: string[];
  email: string | undefined;
  userId: string | undefined;
  phone: string | undefined;
  url: string | undefined;
  ...
}

That way, developers can supply the parameters in any order and it wouldn't break anything and won't have to deal with parameter order gymnastics. I believe Appwrite is written in PHP but I'm sure something like this is also possible.

🎤 Pitch

I was upgrading my SDKs and I was looking at the breaking changes for createMembership() and it's fine that url is now optional but it's frustrating to keep having to check for the order of the parameters. For instance, on version 9.0.0, the order is

createMembership(teamId, roles, url, email, ...)

but on version 11.0.0 the order of parameters got switched all over:

createMembership(teamId, roles, email, userId, phone, url, ...)

Also, now I have to supply a bunch of undefined parameters just to get to the url towards the very end.

I didn't get any indication that something was wrong because Typescript just validates the types and email, userId, and url are all a bunch of string | undefined types so switching them around doesn't trigger any warnings from Typescript.

👀 Have you spent some time to check if this issue has been raised before?

  • I checked and didn't find similar issue

🏢 Have you read the Code of Conduct?

@stnguyen90
Copy link
Contributor

@dokgu, thanks for raising this issue! 🙏🏼 I'm going to move this to the sdk-generator repo since it would apply to the sdk-for-web too.

@stnguyen90 stnguyen90 transferred this issue from appwrite/sdk-for-node Oct 20, 2023
@datner
Copy link

datner commented May 14, 2024

the suggested type

type CreateMembershipProps = {
 // ...
  email: string | undefined;
 // ...
}
createMembership({...}) // Type-Error, `email` is missing
createMembership({..., email: undefined }) // OK

should be

type CreateMembershipProps = {
 // ...
  email?: string
 // ...
}

createMembership({...}) // OK
createMembership({..., email: undefined }) // OK

The former demands a value, either a string or an explicit undefined, the latter does not. The latter gets auto-expanded to email?: string | undefined or stays email?: string with exactOptionalPropertyTypes set to true

@datner
Copy link

datner commented May 14, 2024

I'll attach my comment from the discord verbatim:

the experience using the js (node and web) api is quite painful, there are very obvious C# (and similar) influences in the api to the aggressive detriment of the user experince.. Consider this api:

db.createRelationshipAttribute(
  "<id>",
  "<id>",
  "<id>",
  RelationshipType.OneToMany,
  false,
  "",
  "",
  RelationMutate.Cascade,
)

you can't understand anything reading this, why is the api not the more standard Options interface

db.createRelationshipAttribute({
  databaseId: "<id>",
  parentCollectionId: "<id>",
  childCollectionId: "<id>",
  type: RelationshipType.OneToMany,
  required: false,
  // parentKey?: "",
  // childKey?: "",
  onDelete: RelationMutate.Cascade,
})

or creating namespaced clients

const Parent = new Collection(parentCollection) // <- Models.Collection
Parent.createRelationAttribute({
  to: "<id>",
  type: RelationshipType.OneToMany
})

or even follow the cross-reference style of collection.$id

const Parent = new Collection(parentCollection) // <- Models.Collection
const Child = new Collection(childCollection)
Parent.addRelation(Child, {
  type: RelationshipType.OneToMany
})

This is ofc before the lack of proper types and the recommendation of hard casting results.

I know Appwrite is massive, like MASSIVE massive, both internally and externally with the plethora of sdks, but I really wish the api was more ergonomic
The dream ofc would be to take inspiration from declarative schema-based apis like from kysely or drizzle instead of the imperative approach but I'm trying to keep things realistic

@ajnart
Copy link

ajnart commented Jul 17, 2024

Currently calling the following function :

storage.getFilePreview(
  bucketId, // Required
  fileId,   // Required
  undefined, // width (optional)
  undefined, // height (optional)
  undefined, // gravity (optional)
  undefined, // quality (optional)
  undefined, // borderWidth (optional)
  undefined, // borderColor (optional)
  undefined, // borderRadius (optional)
  undefined, // opacity (optional)
  undefined, // rotation (optional)
  undefined, // background (optional)
  'webp' // output (the only optional parameter you want to set)
);

I really hope this issue gets implemented soon to reduce the complexity of the usage of the web sdk

@Souptik2001
Copy link

Hi @stnguyen90 ,

Do you think this should be done?

Related discord message - https://discordapp.com/channels/564160730845151244/636852860709240842/1263135518871388212

cc: @ajnart

@ajnart
Copy link

ajnart commented Jul 18, 2024

Hi @stnguyen90 ,

Do you think this should be done?

Related discord message - https://discordapp.com/channels/564160730845151244/636852860709240842/1263135518871388212

cc: @ajnart

Cross-posting Steven's discord answer here :

Best thing to do is to gather upvotes. We seem to only get this request for the file preview method so we've been considering adding some utility class or method to make the preview method simpler rather than updating all methods. We're hesitant to update all methods because it would be a big breaking change.

@datner
Copy link

datner commented Jul 20, 2024

Because the sdk is generated, keep the old and add the new. If packed correctly the unused sdk should treeshake out

@dokgu
Copy link
Author

dokgu commented Jul 20, 2024

Hi @stnguyen90 ,
Do you think this should be done?
Related discord message - https://discordapp.com/channels/564160730845151244/636852860709240842/1263135518871388212
cc: @ajnart

Cross-posting Steven's discord answer here :

Best thing to do is to gather upvotes. We seem to only get this request for the file preview method so we've been considering adding some utility class or method to make the preview method simpler rather than updating all methods. We're hesitant to update all methods because it would be a big breaking change.

I do think this is a terrible reason not to do it though - I agree that it is a big breaking change but DX is king for products like Appwrite, if DX is bad developers will look elsewhere. This could be part of a major version upgrade, right now we're at 1.5.7 - this could be part of 2.0.0 since major upgrades often include breaking changes anyway. This would undoubtedly improve DX and Appwrite might get even more market share. Also, I just want to point out that the breaking changes will really only affect existing projects, for projects created from the version of Appwrite that includes these changes - they won't have to deal with any of it at all. We have to think about how we want Appwrite to move forward as it continues to grow.

Another thing to point out is regarding the utility class or method to make the preview method simpler rather than updating all methods - does this mean the methods are inconsistently implemented? That would be a terrible DX if I'm being honest. We should implement methods in a consistent manner so developers won't get tripped up by something unexpected simply because one method is used a different way than the rest.

@ajnart
Copy link

ajnart commented Jul 20, 2024

I do think this is a terrible reason not to do it though - I agree that it is a big breaking change but DX is king for products like Appwrite, if DX is bad developers will look elsewhere.

I couldn't agree more, I checked out the supabase file upload, their approach made sense,I tried to understand why this was not the case with Appwrite and that's how I ended up on this issue

@datner
Copy link

datner commented Jul 21, 2024

+1
I am very regretfully finding myself rewriting the entire sdk so it's usable and I am debating with my partners if we should just ditch appwrite for a "more mature product" (their words) despite the product existing and providing massive value for over 4 years now iirc. I really don't want to use supabase again

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

No branches or pull requests

5 participants