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

Minor fixes #17

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

Minor fixes #17

wants to merge 2 commits into from

Conversation

Paradoxu
Copy link

Basic changes

  • Added comments for better documentation of the SoftDeleteModel interface
  • Added softDeleteById and restoreById for easier manipulation of single documents
  • Replaced Record<...> with mongoose.FilterQuery<...> for better self code documentation and linting
  • Errors should be handled by the callee, which is why I have added an optional onFail parameter so that the callee can decide how to handle errors.

Removed find and save

The previous code would use a query to find all documents that matches the given query and loop through each document, either delete/restore them and call save, this operation can be slow specially when running a query against many documents. For better performance and code readability I have added a simple updateMany for both the restore and softDelete methods, the return value will still be the number of modifiedDocuments by the query.

Exec

The previous code would not call exec when building a query a rellying entiry on the async/await which worked, but for safety reasons it's best to call exec when we actually intend to run the query against the database.

Context

These changes have been made to improve performance, code readability, and maintainability.

export interface SoftDeleteModel<T extends Document> extends mongoose.Model<T> {
findDeleted(): Promise<T[]>;
restore(query: Record<string, any>): Promise<{ restored: number }>;
softDelete(query: Record<string, any>, options?: SaveOptions): Promise<{ deleted: number }>;
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if anyone is using the options parameter now.
this could be a breaking change

Copy link
Author

Choose a reason for hiding this comment

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

Should we consider a major?

Copy link
Contributor

Choose a reason for hiding this comment

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

yea, the majority is important.
In this case, if we bump the package version accordingly to introduce the changes, it should be fine.

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.

None yet

2 participants