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

Drop usage of chrome.storage (or make the storage backend pluggable) #476

Merged
merged 1 commit into from
Jun 20, 2018

Conversation

EirikBirkeland
Copy link
Contributor

The browser version assumes that chrome.storage.local uses an API which is compatible with localStorage; which is not the case. Even though I am using chrome.storage.local for my Chrome extension, I would much prefer to keep debug's variable in localStorage, as I consider chrome.storage.local the 'private space' of my extension.

This change obviates the need to support multiple storage types. But if storage type is important, how about supporting a custom storage facility, including chrome.storage.sync? I.e. the user would provide an object that follows the conventions. Just a thought - I certainly don't see the need at this point though.

The browser version assumes that chrome.storage.local uses an API which is compatible with localStorage; which is not the case. Even though I am using chrome.storage.local for my Chrome extension, I would much prefer to keep debug's variable in localStorage, as I consider chrome.storage.local the 'private space' of my extension.

This change obviates the need to support multiple storage types. But if storage type is important, how about supporting a custom storage facility, including chrome.storage.sync? I.e. the user would provide an object that follows the conventions. Just a thought - I certainly don't see the need at this point though.
@coveralls
Copy link

coveralls commented Jul 17, 2017

Coverage Status

Coverage remained the same at 63.804% when pulling da51af8 on EirikBirkeland:patch-1 into a45d4a0 on visionmedia:master.

@thebigredgeek
Copy link
Contributor

thebigredgeek commented Aug 4, 2017

I think the idea of using a "default" storage type and providing a way to "plug in" your own is a worthy idea. For now, though, this is a breaking change for existing extension support as the storage wouldn't be extension-specific.

@TooTallNate TooTallNate changed the title Simplify and improve Drop usage of chrome.storage (or make the storage backend pluggable) Aug 8, 2017
@EirikBirkeland
Copy link
Contributor Author

EirikBirkeland commented Oct 29, 2017

I see your concerns.
But actually, at least in my experience, extension support is already broken, so removing chrome.storage.local would actually be a great thing.
The reason is that chrome.storage.* uses an async API not compatible with localStorage, so it's not pluggable unless we create a universal storage wrapper that then communicates correctly with whatever storage mechanism preferred by the user.

Also, chrome.storage.local requires a manifest.json permission. I find it rather surprising that any NPM module would avail itself of that facility without notifying me or without implementing it as an option.

Therefore, IMHO, removing it and keeping localStorage / env variable as the built-in solution would be the best option. This would not be a breaking change, since chrome.storage.local support is already broken for those developing Chrome extensions and using chrome.storage.

I suppose that having chrome.storage.local support would be useful for syncing debug behavior across websites and background, but personally I have no issues with manually adding the localStorage variable in 2 locations. localStorage also has the advantage of persisting across extension installs, which is not the case for chrome.storage.*

Please see the Chrome Storage API reference to see how it's not localStorage-compatible:
https://developer.chrome.com/extensions/storage

@hdodov
Copy link

hdodov commented May 16, 2018

Please merge, you guys. Using chrome.storage.local incorrectly makes the module completely useless when you have the storage permission enabled in your extension.

@EirikBirkeland
Copy link
Contributor Author

EirikBirkeland commented May 16, 2018

tl;dr;
I confirm. We need to drop Crome extension support, because it's broken. The interface is invalid. Please have a look at the original PR code contents.

Thank you for a great tool, we're always using it!

Copy link
Member

@Qix- Qix- left a comment

Choose a reason for hiding this comment

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

Agreed - while this might have caused problems at one point this seems to be pretty ubiquitous now.

https://caniuse.com/#search=localStorage

@Qix- Qix- added the bug This issue identifies a malfunction label Jun 20, 2018
@Qix- Qix- merged commit 71d2aa7 into debug-js:master Jun 20, 2018
@Qix-
Copy link
Member

Qix- commented Jun 20, 2018

Thanks @EirikBirkeland!

@EirikBirkeland EirikBirkeland deleted the patch-1 branch June 20, 2018 19:35
@lidel
Copy link

lidel commented Aug 26, 2018

Will there be a release with this fix?
Last one is 3.1.0 from 2017 :)

@lidel lidel mentioned this pull request Aug 26, 2018
1 task
@EirikBirkeland
Copy link
Contributor Author

EirikBirkeland commented Aug 27, 2018

I'd fork or pin package to Github commit :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue identifies a malfunction
Development

Successfully merging this pull request may close these issues.

6 participants