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

Allow using chrome.storage.local #60

Merged
merged 7 commits into from
Apr 15, 2022
Merged

Allow using chrome.storage.local #60

merged 7 commits into from
Apr 15, 2022

Conversation

Stvad
Copy link
Contributor

@Stvad Stvad commented Apr 7, 2022

I use it in https://github.com/transclude-me/extension

Motivation: I started using this via template, but apparently the values for settings are too large to store remotely 😿 . I still like the interface/form sync aspects of this and so, substituting the storage area to be local storage seems like a good solution.

Prior interest: #19


Question/confusion I have here - this lib is using the the chrome* api*s and yet works with FF, and supplying browser.storage.local works for passing the stoarge area here, but presumably this has different apis (callback vs promise). How does interop work here?

Stvad added a commit to transclude-me/extension that referenced this pull request Apr 7, 2022
@Stvad
Copy link
Contributor Author

Stvad commented Apr 8, 2022

So it seems my confusion is not unwarranted, passing in browser.storage.* works in FF but not in Chrome (??). Theoretically one can pass chrome.storage* but that seems counterinuitive in crossbrowser extension. So thinking of:

  • pass string storage area type instead
  • migrate the library to use webext-polyfill and promise api's, which would allow to preserve object passing interface and probably a good thing to do anyhow. It's a bit of a larger change though 🤔

Lmk if you have preferences between those 2!

@Stvad
Copy link
Contributor Author

Stvad commented Apr 8, 2022

Ok, went with option 2 as it seemed simple enough

@fregante
Copy link
Owner

fregante commented Apr 8, 2022

Thank you for the PR! I'd rather not include the polyfill, also because this change won't work if users pass a raw chrome.storage.local object (which isn't promised on Manifest v2 in Chrome).

You can pass a string instead

@Stvad
Copy link
Contributor Author

Stvad commented Apr 8, 2022

I'd rather not include the polyfill,

huh, I'm curious why? I was really surprised this using chrome* as it is aiming to be cross-browser (and a bit surprised it works in FF tbh :p )

@fregante
Copy link
Owner

fregante commented Apr 8, 2022

huh, I'm curious why?

Because it's as large as this package + it's not necessary (webext-options-sync has worked fine without it since its inception) + the polyfill won't be deduplicated across dependencies because they release a "server breaking" version every time.

If I really need promise API, I made a 300-byte wrapper, but that's still not required for this change.

I was really surprised this using chrome* as it is aiming to be cross-browser

chrome.* APIs are the cross browser APIs because Chrome only supports those. Every other browser has just been playing catch up and have to support that, even if they push a more generic browser.* API.

@Stvad
Copy link
Contributor Author

Stvad commented Apr 8, 2022

Pushed an update to use the string type

Stvad added a commit to transclude-me/extension that referenced this pull request Apr 9, 2022
index.ts Show resolved Hide resolved
index.ts Outdated
@@ -32,6 +32,8 @@ async function shouldRunMigrations(): Promise<boolean> {
});
}

export type StorageType = 'sync' | 'local' | 'managed';
Copy link
Owner

Choose a reason for hiding this comment

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

managed is read-only, it's not supported by webext-options-sync

Suggested change
export type StorageType = 'sync' | 'local' | 'managed';
export type StorageType = 'sync' | 'local';

readme.md Show resolved Hide resolved
readme.md Outdated
@@ -244,6 +244,13 @@ Default: `true`

Whether info and warnings (on sync, updating form, etc.) should be logged to the console or not.

###### storageType

Type: `'loacal' | 'sync' | 'managed'`
Copy link

Choose a reason for hiding this comment

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

Suggested change
Type: `'loacal' | 'sync' | 'managed'`
Type: `'local' | 'sync' | 'managed'`

@Stvad
Copy link
Contributor Author

Stvad commented Apr 14, 2022

checking in! 🙂

Copy link
Owner

@fregante fregante left a comment

Choose a reason for hiding this comment

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

The changes and the readme update look great!

@aspiers
Copy link

aspiers commented Apr 15, 2022

Awesome, thanks so much both - looking forward to trying this!

@fregante fregante changed the title Allow people to supply the StorageArea they want to use for settings sync Allow using chrome.storage.local Apr 15, 2022
@fregante fregante merged commit 65f0a19 into fregante:main Apr 15, 2022
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

3 participants