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

onChanged callback object content clarification #31227

Merged
merged 8 commits into from
Mar 4, 2024

Conversation

rebloor
Copy link
Contributor

@rebloor rebloor commented Dec 22, 2023

Description

Clarifies that the storage.onChanged callback triggered as a result of storageArea.set only returns changed details.

Related issues and pull requests

Fixes #31203

@rebloor rebloor added the Content:WebExt WebExtensions docs label Dec 22, 2023
@rebloor rebloor self-assigned this Dec 22, 2023
Copy link
Contributor

github-actions bot commented Dec 22, 2023

Preview URLs

External URLs (2)

URL: /en-US/docs/Mozilla/Add-ons/WebExtensions/API/storage/StorageArea/onChanged
Title: storage.StorageArea.onChanged


URL: /en-US/docs/Mozilla/Add-ons/WebExtensions/API/storage/onChanged
Title: storage.onChanged

(comment last updated: 2024-03-04 15:46:28)

@erosman
Copy link
Contributor

erosman commented Dec 22, 2023

The issue had to be updated.
Please hold the PR.

Sorry for the inaccurate test results.

@rebloor
Copy link
Contributor Author

rebloor commented Dec 23, 2023

@erosman updated based on your issue revisions

@erosman
Copy link
Contributor

erosman commented Dec 23, 2023

Just a note which may save some developers some time.
Feel free to include something about it or not.

While Firefox storage onChanged (currently, see Bug 1833153) returns all the values and Chrome (currently) only the changed values, the target storage area still contains all the values e.g.

// same for browser and chrome APIs
browser.storage.onChanged.addListener()
browser.storage.sync.onChanged.addListener()

// the following contains all the values passed to the storage sync, changed and unchanged
browser.storage.sync.get()

@rebloor
Copy link
Contributor Author

rebloor commented Dec 25, 2023

@erosman sorry, I'm unclear what the note about get would achieve if the developers were trying to observe changed keys.

@erosman
Copy link
Contributor

erosman commented Dec 25, 2023

@erosman sorry, I'm unclear what the note about get would achieve if the developers were trying to observe changed keys.

When developing a cross-browser-compatible extension, API inconsistency is a major concern. [1]
What I came up with in this case was to use onChanged as a trigger and then get all the values from storage.sync.get(), the same way Firefox returns onChanged.
While that is not necessary in all the cases, it is helpful in some cases (especially when a large values has to be segmented). [2]

I don't know if it is something worth mentioning or not, and feel free to disregard it.

Please also note

  1. Inconsistency: storage onChanged
  2. Proposal: Increase maximum item size in Storage sync quotas

@rebloor rebloor marked this pull request as ready for review February 28, 2024 02:04
@rebloor rebloor requested a review from a team as a code owner February 28, 2024 02:04
@rebloor rebloor requested review from jpmedley and removed request for a team February 28, 2024 02:04
@@ -7,7 +7,9 @@ browser-compat: webextensions.api.storage.StorageArea.onChanged

{{AddonSidebar()}}

Fires when one or more items in a storage area change. Compared to {{WebExtAPIRef("storage.onChanged")}}, this event enables you to listen for changes in one of the storage areas: `local`, `managed`, `session`, and `sync`.
Fires when one or more items in a storage area change, returning details for the keys that changed. Compared to {{WebExtAPIRef("storage.onChanged")}}, this event enables you to listen for changes in one of the storage areas: `local`, `managed`, `session`, and `sync`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

So, Firefox can pass changes for multiple areas, but everyone else only does one at a time? Do I understand this correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jpmedley, as I understand it, the difference is that Firefox returns all the keys from the storage area, whereas other browsers only return the changed ones only.

Co-authored-by: Joe Medley <jmedley@google.com>
@github-actions github-actions bot added the size/s [PR only] 6-50 LoC changed label Mar 1, 2024
@rebloor rebloor requested a review from jpmedley March 1, 2024 02:23
Copy link
Collaborator

@jpmedley jpmedley left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. I had a long weekend. I don't usually request multiple passes of changes. I'm making an exception because I think we need to clarify the difference between Firefox and other browsers.

Co-authored-by: Joe Medley <jmedley@google.com>
@rebloor rebloor requested a review from jpmedley March 4, 2024 15:44
@jpmedley jpmedley merged commit b9e1bf1 into mdn:main Mar 4, 2024
9 checks passed
@rebloor rebloor deleted the onChanged-object-content-clarification branch March 4, 2024 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:WebExt WebExtensions docs size/s [PR only] 6-50 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Conflicting Information regarding storage onChanged
3 participants