feat: strongly typed item methods #1659

tranhl · 2024-02-03T07:14:21Z

Summary:

This PR adds support for strongly-typed custom item methods via a second generic type parameter to dynamoose.model.

Code sample:

// 1. Create a custom item method type that extends from `ItemMethods`.
// This is ensures that only functions are present in the type.
interface UserItemMethods extends ItemMethods {
	resetPassword: () => Promise<void>;
}

// 2. Create a type for `Item` as usual.
interface UserItem extends Item {
	id: string;
	name: string;
	age: number;
}

// 3. Create a schema.
const userSchema = new dynamoose.Schema({
	"id": String,
	"name": {
		"type": String,
		"index": {
			"type": IndexType.global
		}
	},
	"age": Number
});

// 4. Pass item & custom method types to `dynamoose.model`
const UserModel = dynamoose.model<UserItem, UserItemMethods>(
	"User",
	userSchema
);

const user = await UserModel.get('id');
// 5. Access item methods with type safety.
user.resetPassword() // () => Promise<void>

GitHub linked issue:

Implements part of #1647

Type (select 1):

Is this a breaking change? (select 1):

🚨 YES 🚨
No
I'm not sure

Is this ready to be merged into Dynamoose? (select 1):

Yes
No

Are all the tests currently passing on this PR? (select 1):

Yes
No

Other:

I have read through and followed the Contributing Guidelines
I have searched through the GitHub pull requests to ensure this PR has not already been submitted
I have updated the Dynamoose documentation (if required) given the changes I made
I have added/updated the Dynamoose test cases (if required) given the changes I made
I agree that all changes made in this pull request may be distributed and are made available in accordance with the Dynamoose license
All of my commits and commit messages are detailed, explain what changes were made, and are easy to follow and understand
I have filled out all fields above

github-actions · 2024-02-03T07:14:36Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

tranhl · 2024-02-03T07:24:41Z

@fishcharlie Hoping to get your input on my design/implementation of strongly typed item methods. I've created a draft PR for now just in case the design needs rework. Once you're happy with the PR I'll go ahead and update documentation & mark it as ready.

I've done the best I can to make sure that anywhere an Item is returned, the ItemMethods are now also being returned as well (e.g. after executing a Scan or Query operation). However, my understanding of the public interface is not exhaustive so do point out if I've missed anything.

tranhl · 2024-02-03T07:24:45Z

I dedicate any and all copyright interest in this software to the public domain. I make this dedication for the benefit of the public at large and to the detriment of my heirs and successors. I intend this dedication to be an overt act of relinquishment in perpetuity of all present and future rights to this software under copyright law.

tranhl · 2024-02-03T07:34:07Z

packages/dynamoose/lib/Model/index.ts

@@ -439,21 +439,21 @@ export class Model<T extends ItemCarrier = AnyItem> extends InternalPropertiesCl
 	// latestTableDetails: DynamoDB.DescribeTableOutput;
 	// pendingTaskPromise: () => Promise<void>;
 	Item: typeof ItemCarrier;


@fishcharlie Is being able to access the underlying Item class from a model part of public interface? If it is not, then there aren't any issues. However if it is, then we'll have to figure out a way to better type this property.

For example, this is possible:

const UserModel = dynamoose.model('User', userSchema) const user = new UserModel.Item({ id: 'foobar' })

This works fine in JS, however typing this correctly in TS has proven challenging. I made a naive attempt at fixing the types:

Suggested change

Item: typeof ItemCarrier;

Item: T & typeof ItemCarrier;

However, TypeScript understandably isn't happy with this, as the internal Item class created within Model is not assignable to the generic we pass in:

Type 'typeof Item' is not assignable to type 'T & typeof Item'. Type 'typeof Item' is not assignable to type 'T'. 'T' could be instantiated with an arbitrary type which could be unrelated to 'typeof Item'.

Haven't been able to find a good solution to this yet.

@tranhl

Is being able to access the underlying Item class from a model part of public interface?

It might work. But it's not supported. If someone tries to do that in their code I would point them to this: https://dynamoosejs.com/other/FAQ#can-i-use-an-undocumented-property-class-method-or-function-in-dynamoose.

Okay perfect 👍 .

It's also mentioned in the TS docs that documentation should be treated as the source of truth, so we can set this aside until Model.Item becomes public interface.

fishcharlie

Overall this looks really good.

Beyond the code level comments I made the only other high level suggestion is that we need documentation for this. All the documentation either exists in the code itself using JSDoc or in docs/docs_src.

fishcharlie · 2024-02-04T21:24:21Z

packages/dynamoose/lib/General.ts

@@ -14,7 +14,9 @@ export type KeyObject = {[attribute: string]: string | number};
 // An item representing a DynamoDB key
 export type InputKey = string | number | KeyObject;

-interface ModelItemConstructor<T extends Item> {
+type Expand<T> = {[KeyType in keyof T]: T[KeyType]} & {};


Why the & {}?

Also, can you give an example of what this would look like? For some reason this type seems very complex at first glance. It's not obviously clear what it's doing and what it's purpose is. Maybe add a comment above with an example or something? Or a better description?

Also should this be in a separate file?

Good points! This type definitely could use a JSDoc explaining its function & intended use. Also agree that it should be moved to a separate file. Best location for it from what I can see would be lib/Types.ts. How does that sound?

lib/Types.ts works for me 🎉

fishcharlie · 2024-02-04T21:28:06Z

packages/dynamoose/lib/Item.ts

+export interface AnyItemMethods {
+	[key: string]: any;
+}


It's not immediately clear what this is doing. Seems more general than just AnyItemMethods. Why do we have to use any vs Function? If it's item methods, shouldn't that be a function?

Fair point. I encountered an issue with TS early on omitting the ItemMethods generic when calling dynamoose.model, however I believe that requiring custom methods to extend ItemMethods addresses this. Let me revisit this and see if it's possible to completely remove it.

fishcharlie · 2024-02-04T21:29:54Z

packages/dynamoose/lib/Model/index.ts

@@ -439,21 +439,21 @@ export class Model<T extends ItemCarrier = AnyItem> extends InternalPropertiesCl
 	// latestTableDetails: DynamoDB.DescribeTableOutput;
 	// pendingTaskPromise: () => Promise<void>;
 	Item: typeof ItemCarrier;


@tranhl

Is being able to access the underlying Item class from a model part of public interface?

It might work. But it's not supported. If someone tries to do that in their code I would point them to this: https://dynamoosejs.com/other/FAQ#can-i-use-an-undocumented-property-class-method-or-function-in-dynamoose.

fishcharlie · 2024-02-04T21:32:07Z

packages/dynamoose/test/types/Item.ts

@@ -11,3 +11,5 @@ const shouldPassSaveWithReturnItemCallback = user.save({"return": "item"}, () =>

 // @ts-expect-error
 const shouldFailWithInvalidReturnType = user.save({"return": "invalid-return-type"});
+
+const shouldPassCustomMethodAccess = user.resetPassword();


Maybe we should also have a test with a method that doesn't exist? We can use // @ts-expect-error for that one.

Definitely. Test coverage across edge cases was minimal as I was unsure if you'd be happy with the approach of the PR. Will update/refactor tests & documentation to be more comprehensive.

fishcharlie · 2024-02-04T21:33:33Z

packages/dynamoose/test/types/Model.ts

@@ -89,7 +95,7 @@ const userSchema = new dynamoose.Schema({
 	"age": Number
 });

-export const UserTypedModel = dynamoose.model<User>(
+export const UserTypedModel = dynamoose.model<User, UserMethods>(


What happens here if you don't pass in UserMethods? Does it still work? I kinda think we should be breaking this out into two different tests. The existing tests are just fine. We should have new tests for these UserMethods that don't modify the old tests.

As part of the interface, it should work. As per my previous comment, I will make sure this is covered in tests.

fishcharlie · 2024-02-04T21:34:18Z

packages/dynamoose/test/types/Model.ts

@@ -110,3 +116,6 @@ UserTypedModel.update({"id": "foo"}, {
 UserTypedModel.update({"id": "foo"}, {
 	"$REMOVE":{"age":null}
 });
+
+// @ts-expect-error
+const shouldFailItemCustomMethodAccess = UserTypedModel.resetPassword();


So are model methods outside the scope of this PR? This PR only deals with item methods it looks like?

That's correct, I've decided to address them separately to keep scope focused. Happy to include model methods as well if that suits you?

@tranhl Ideally I'd like to review both PRs before merging either of them. I don't want to be left in a state where one piece is done and people ask why the other piece wasn't implemented.

You can decide how to best structure it. I'd personally prefer 2 separate PRs so that it's easier to review and the context is easier to understand. Sadly I don't think GitHub has the concept of having one PR depend on another.

github-actions · 2024-02-13T01:18:49Z

This pull request is stale because it has been open 7 days with no activity. Remove stale label or comment or this will be closed in 3 days.

jimmyn · 2024-03-22T09:53:04Z

It would be great to have this feature live!

tranhl force-pushed the feat/strongly-typed-model-item-methods branch from 1446a81 to 107cd29 Compare February 3, 2024 07:19

tranhl commented Feb 3, 2024

View reviewed changes

github-actions bot added a commit that referenced this pull request Feb 3, 2024

@tranhl has signed the CLA from Pull Request #1659

ba8d0f0

fishcharlie requested changes Feb 4, 2024

View reviewed changes

github-actions bot added the status:stale label Feb 13, 2024

fishcharlie added status:not-stale and removed status:stale labels Feb 13, 2024

tranhl added 4 commits March 25, 2024 00:44

feat: strongly typed item methods

f0bfb1f

refactor Expand type into lib/Types.ts

858e5d9

remove redundant AnyItemMethods

f825906

test: fail on unknown typed method access

f13f9d5

tranhl force-pushed the feat/strongly-typed-model-item-methods branch from 0d570ee to f13f9d5 Compare March 24, 2024 13:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: strongly typed item methods #1659

feat: strongly typed item methods #1659

tranhl commented Feb 3, 2024

github-actions bot commented Feb 3, 2024 •

edited

tranhl commented Feb 3, 2024 •

edited

tranhl commented Feb 3, 2024 •

edited

tranhl Feb 3, 2024

fishcharlie Feb 4, 2024

tranhl Feb 5, 2024

fishcharlie left a comment

fishcharlie Feb 4, 2024

fishcharlie Feb 4, 2024

tranhl Feb 5, 2024

fishcharlie Feb 5, 2024

fishcharlie Feb 4, 2024

tranhl Feb 5, 2024

fishcharlie Feb 4, 2024

fishcharlie Feb 4, 2024

tranhl Feb 5, 2024 •

edited

fishcharlie Feb 4, 2024

tranhl Feb 5, 2024

fishcharlie Feb 4, 2024

tranhl Feb 5, 2024

fishcharlie Feb 5, 2024

github-actions bot commented Feb 13, 2024

jimmyn commented Mar 22, 2024

feat: strongly typed item methods #1659

Are you sure you want to change the base?

feat: strongly typed item methods #1659

Conversation

tranhl commented Feb 3, 2024

Summary:

Code sample:

GitHub linked issue:

Type (select 1):

Is this a breaking change? (select 1):

Is this ready to be merged into Dynamoose? (select 1):

Are all the tests currently passing on this PR? (select 1):

Other:

github-actions bot commented Feb 3, 2024 • edited

tranhl commented Feb 3, 2024 • edited

tranhl commented Feb 3, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fishcharlie 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

tranhl Feb 5, 2024 • edited

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

github-actions bot commented Feb 13, 2024

jimmyn commented Mar 22, 2024

github-actions bot commented Feb 3, 2024 •

edited

tranhl commented Feb 3, 2024 •

edited

tranhl commented Feb 3, 2024 •

edited

tranhl Feb 5, 2024 •

edited