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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃悰 Bug Report: Document autocompletion for Databases.createDocument broken #76

Open
2 tasks done
emmiep opened this issue Sep 29, 2023 · 3 comments
Open
2 tasks done
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@emmiep
Copy link

emmiep commented Sep 29, 2023

馃憻 Reproduction steps

When using an IDE/editor supporting autocompletion with the Typescript Language Server, such as VS Code.

Calling Databases.createDocument with a type parameter:

interface ProjectModel extends Models.Document {
  name: string;
  whatever: number;
}

/* ... */

const project = await databases.createDocument<ProjectModel>(
  databaseId,
  collectionId,
  ID.unique(),
  {
    name: "my project",
  },
});

馃憤 Expected behavior

I should see the properties I defined in the ProjectModel interface when VS Code autocompletes the code inside the curly brackets.

馃憥 Actual Behavior

VS Code shows identifiers from the function scope, but not the properties defined in the ProjectModel interface:

SCR-20230929-q7u

Possible solution

I think this is caused by the data argument of Databases.createDocument having the type Omit<Document, keyof Models.Document>.

Since Models.Document contains a string index signature, the value of keyof Models.Document is probably string | number. That would remove all properties from the Document type parameter, basically turning it into an empty object type ({}).

馃幉 Appwrite version

Different version (specify in environment)

馃捇 Operating system

MacOS

馃П Your Environment

Appwrite Cloud

Appwrite web SDK version:

13.0.0

VS Code version information:

Version: 1.82.2 (Universal)
Commit: abd2f3db4bdb28f9e95536dfa84d8479f1eb312d
Date: 2023-09-14T05:59:47.790Z (2 wks ago)
Electron: 25.8.1
ElectronBuildId: 23779380
Chromium: 114.0.5735.289
Node.js: 18.15.0
V8: 11.4.183.29-electron.0
OS: Darwin arm64 22.5.0

MacOS version:

13.4.1 (22F82)

馃憖 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?

@emmiep emmiep added the bug Something isn't working label Sep 29, 2023
@emmiep
Copy link
Author

emmiep commented Sep 29, 2023

I'm not sure why the Models.Document type needs the index signature, is this a thing to make sure people who use JS/don't use the type parameter don't get any warnings when accessing custom attributes from the documents?

Personally, I want the type safety of Typescript, so I would prefer if the Document base type didn't include that.
Is it possible to remove this from the Document, then maybe creating an AnyDocument type which could be the default type parameter value?

/* models.ts */

export type Document = {
  $id: string;
  $collectionId: string;
  $databaseId: string;
  $createdAt: string;
  $updatedAt: string;
  $permissions: string[];
};

export interface AnyDocument extends Document {
  [key: string]: any;
}

/* databases.ts */

async createDocument<Document extends Models.Document = Models.AnyDocument>(
  databaseId: string,
  collectionId: string,
  documentId: string,
  data: Omit<Document, keyof Models.Document>,
  permissions?: string[],
): Promise<Document> {
  /* ... */
}

That would probably solve this issue as well.

@stnguyen90
Copy link
Contributor

@emmiep,

I'm not sure why the Models.Document type needs the index signature, is this a thing to make sure people who use JS/don't use the type parameter don't get any warnings when accessing custom attributes from the documents?

Correct, this is to prevent errors when someone just calls createDocument() without passing in their own type.

Personally, I want the type safety of Typescript, so I would prefer if the Document base type didn't include that. Is it possible to remove this from the Document, then maybe creating an AnyDocument type which could be the default type parameter value?

/* models.ts */

export type Document = {
  $id: string;
  $collectionId: string;
  $databaseId: string;
  $createdAt: string;
  $updatedAt: string;
  $permissions: string[];
};

export interface AnyDocument extends Document {
  [key: string]: any;
}

/* databases.ts */

async createDocument<Document extends Models.Document = Models.AnyDocument>(
  databaseId: string,
  collectionId: string,
  documentId: string,
  data: Omit<Document, keyof Models.Document>,
  permissions?: string[],
): Promise<Document> {
  /* ... */
}

This is a great idea! We generate our SDKs using our sdk-generator and the template for the [key: string]: any; is here. The tough thing would be to figure out how to update the SDK generator to support what you're suggesting 馃

@stnguyen90 stnguyen90 self-assigned this May 3, 2024
@stnguyen90 stnguyen90 added the help wanted Extra attention is needed label May 3, 2024
@stnguyen90
Copy link
Contributor

Opening this for contributors to see if they can update the SDK generator to add this behavior.

@stnguyen90 stnguyen90 removed their assignment May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants