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

fix: Storage.prototype.remove and Storage.prototype.removeAll to handle namespace correctly #32

Merged
merged 4 commits into from
May 10, 2023

Conversation

shuntksh
Copy link
Contributor

@shuntksh shuntksh commented May 7, 2023

Details

fixes: #31

This PR addresses an issue in the @plasmo/storage where the remove method does not handle namespaces correctly. It also introduces new test cases that cover basic CRUD operations.

Changes:

  • Updated the Storage class in the @plasmo/storage package to correctly handle namespaces when performing the remove operation.
  • Added new test cases that utilize a modified mock to verify basic CRUD operations, ensuring proper functionality and increasing test coverage.

Code of Conduct

Contacts

  • (OPTIONAL) Discord ID:

If your PR is accepted, we will award you with the Contributor role on Discord server.

To join the server, visit: https://www.plasmo.com/s/d

@louisgv
Copy link
Contributor

louisgv commented May 8, 2023

@shuntksh is this PR good to go? Since it's in draft state I'm assumed you might wanted to add some more. Since we're moving the remove method into abstract, we will also need to add that to SecureStorage as well right

@shuntksh shuntksh marked this pull request as ready for review May 8, 2023 15:31
@shuntksh
Copy link
Contributor Author

shuntksh commented May 8, 2023

Hi @louisgv, I appreciate your input! I've added the remove method to SecureStorage and created a basic CRUD test to ensure any future changes will identify any inconsistencies in the implementation. And I published the PR for your review.

Copy link
Contributor

@louisgv louisgv left a comment

Choose a reason for hiding this comment

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

Great finding! @shuntksh - I migrated the setup script to typescript so we can avoid those ts-ignore :D Thank you for your contribution.

@louisgv louisgv merged commit 3a25b34 into PlasmoHQ:main May 10, 2023
@shuntksh
Copy link
Contributor Author

Thanks, @louisgv for taking the time to review the change! TIL "mts" to write a setup file in TypeScript (with ts-jest)! Good to know!

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.

storage.delete does not seem to use Namespace causing entries not to be deleted.
2 participants