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

storage.delete does not seem to use Namespace causing entries not to be deleted. #31

Closed
shuntksh opened this issue May 6, 2023 · 3 comments · Fixed by #32
Closed

storage.delete does not seem to use Namespace causing entries not to be deleted. #31

shuntksh opened this issue May 6, 2023 · 3 comments · Fixed by #32

Comments

@shuntksh
Copy link
Contributor

shuntksh commented May 6, 2023

Hi Plasmo team,

Not sure if this is an intended behavior but Storage.prototype.remove does not seem to consider the namespace, causing the workers that are using the namespace to fail to remove items with corresponding keys.

src/index.ts

await this.#primaryClient.remove(key)

Should this also use getNamespacedKey(key) like get and set and have a rawRemove to remove entries directly?

  remove = async (key: string) => {
    const nsKey = getNamespacedKey(key)
    if (this.isCopied(nsKey)) {
      this.#secondaryClient?.removeItem(nsKey)
    }

    if (this.hasExtensionApi) {
      await this.#primaryClient.remove(nsKey)
    }
  }

I'd also be happy to work on a fix if this is unintended.

@louisgv
Copy link
Contributor

louisgv commented May 7, 2023

@shuntksh Thank you so much for the issue! This is indeed unintended - the namespace feature was made quickly to experiment with the concept, and I indeed missed some of the places that need to incorporate the namespace along the way xD

I'd also be happy to work on a fix if this is unintended.

That would be greatly appreciated - thank you 🙏

@shuntksh
Copy link
Contributor Author

shuntksh commented May 7, 2023

Thanks, @louisgv! I've submitted a PR to the main repository. I'm not certain if that's the right approach, so please let me know if you'd prefer me to open a PR in this repository instead. Also, if there's anything I missed in the changes, just let me know!

Here's the link to the PR: PlasmoHQ/plasmo#573

@shuntksh
Copy link
Contributor Author

shuntksh commented May 7, 2023

@louisgv - Sorry I realized the original PR did not include the change I made to the sub-module, as it is pointing to the wrong repository. I reopened it as a draft PR in this repository. Let me know if this is the correct repository for the change.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants