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

Should browser.storage.local.set({"": ""}) and browser.storage.local.get("") be valid? #278

Closed
xeenon opened this issue Sep 16, 2022 · 12 comments
Labels
implemented: chrome Implemented in Chrome implemented: firefox Implemented in Firefox inconsistency Inconsistent behavior across browsers spec clarification Needs clarification when specified supportive: safari Supportive from Safari

Comments

@xeenon
Copy link
Collaborator

xeenon commented Sep 16, 2022

Currently Chrome supports doing browser.storage.local.set({ "": "foo" }) and browser.storage.local.get(""). We always return an empty result object in Safari in if the key is empty. We have found an extension in the wild doing this, and will likely change Safari to match Chrome.

@xeenon xeenon added inconsistency Inconsistent behavior across browsers spec clarification Needs clarification when specified implemented: chrome Implemented in Chrome supportive: safari Supportive from Safari labels Sep 16, 2022
@xeenon xeenon changed the title Should browser.storage.local.set("") and browser.storage.local.get("") be valid? Should browser.storage.local.set({"": ""}) and browser.storage.local.get("") be valid? Sep 16, 2022
@bershanskiy
Copy link
Member

For reference, Firefox also supports this (matches Chrome).

We have found an extension in the wild doing this

This is interesting, is this extension public/in the store? Does it appear accidental or intentional?

@carlosjeurissen
Copy link
Contributor

In any case, as we talked about in TPAC, for developers it would be very good to know about this behaviour in Safari even if it will be changed in the future. Mentioning it on this mdn page would be great: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/storage/StorageArea/set

Generally speaking an empty key is just valid in JSON, as can be seen in the standard:
https://www.ecma-international.org/publications-and-standards/standards/ecma-404/
"The JSON syntax does not impose any restrictions on
the strings used as names". And as can be seen in below figure, an empty key can be used without issue.

That plus the fact Google Chrome and Firefox already support this, it seems best to change the behaviour in Safari as well.

string-ecma

@bershanskiy
Copy link
Member

bershanskiy commented Sep 17, 2022

Mentioning it on this mdn page would be great:

I decided to add this info to BCD and found the following curious detail:

browser.storage.local.set({"":"example"})
browser.storage.local.get("", (a) => console.log(a[""]))

logs out undefined

However,

browser.storage.local.set({"":"example"})
browser.storage.local.get(null, (a) => console.log(a[""]))

logs out example

And

browser.storage.local.set({"":"example"})
browser.storage.local.get([""], (a) => console.log(a[""]))

throws Error: Invalid empty key found in array passed to storage.local.get()

And

browser.storage.local.remove("")
browser.storage.local.remove([""])

both throw Error: Invalid empty key found in array passed to storage.local.remove()

In other words, this is just a difference in querying code.

I'll add a note to MDN BCD.

Edit: I updated the examples a bit. Edit 2: added remove()

@carlosjeurissen
Copy link
Contributor

@bershanskiy This was tested in Mozilla Firefox?

I have tested these cases in Google Chrome, and in Google Chrome they just return in all three cases like you would expect.

@bershanskiy
Copy link
Member

@bershanskiy This was tested in Mozilla Firefox?
I have tested these cases in Google Chrome, and in Google Chrome they just return in all three cases like you would expect.

I tested those in Firefox (and, just for more confidence, on Chrome after replacing browser with chrome) and it works as expected in both.

I believe that Safari just has a bug in browser.storage.local.get() (while browser.storage.local.set() can save data with empty keys).

@xeenon
Copy link
Collaborator Author

xeenon commented Sep 20, 2022

Yes, browser.storage.local.get() is the broken part in Safari. We are working on fixing it. Thanks for adding a note to MDN!

@Rob--W
Copy link
Member

Rob--W commented Sep 22, 2022

@bershanskiy This was tested in Mozilla Firefox?
I have tested these cases in Google Chrome, and in Google Chrome they just return in all three cases like you would expect.

I tested those in Firefox (and, just for more confidence, on Chrome after replacing browser with chrome) and it works as expected in both.

Adding implemented: firefox because of this.

@Rob--W Rob--W added the implemented: firefox Implemented in Firefox label Sep 22, 2022
@carlosjeurissen
Copy link
Contributor

Since we all agreed on the preferred behaviour, there seems to be no work left for this issue within the group. Should we close the issue in this case? Or do we want to keep it open until it has been implemented by Safari?

In any case, @xeenon can we add a note somewhere to update https://github.com/bershanskiy/browser-compat-data/blob/main/webextensions/api/storage.json under "empty_key", and change the "version_added" once this has been fixed by Safari?

@Rob--W
Copy link
Member

Rob--W commented Sep 29, 2022

Since we all agreed on the preferred behaviour, there seems to be no work left for this issue within the group. Should we close the issue in this case? Or do we want to keep it open until it has been implemented by Safari?

IMO it's fine to close since we've completed the discussion on interoperability. But I'll leave it up to @xeenon to close this, in case having it open is helpful.

In any case, @xeenon can we add a note somewhere to update https://github.com/bershanskiy/browser-compat-data/blob/main/webextensions/api/storage.json under "empty_key", and change the "version_added" once this has been fixed by Safari?

FYI the canonical repo is mdn/content, i.e. https://github.com/mdn/browser-compat-data/blob/main/webextensions/api/storage.json

I'm not sure if we need to list this in the BCD. This is an implementation bug for an uncommon scenario. I'm not opposed to documenting it, but I do wonder where we draw the line between an eventually obsolete historical note and an incompatibility that's worth calling out.

Technically, partial_implementation could be used, but that feels a bit too heavy for such an edge case. (https://github.com/mdn/browser-compat-data/blob/main/docs/data-guidelines/index.md#partial_implementation-requires-a-note)

@xeenon xeenon closed this as completed Oct 2, 2022
@lapcat
Copy link

lapcat commented Oct 13, 2022

For reference, Firefox also supports this (matches Chrome).

We have found an extension in the wild doing this

This is interesting, is this extension public/in the store? Does it appear accidental or intentional?

I just stumbled upon this issue, and I suspect that it's referring to my own extension, considering that I filed a Feedback with Apple on the issue 6 weeks ago. The behavior is intentional. The keys represent site-specific options, and an empty key is used for the defaults.

I have a workaround for Safari that I don't need for Chrome or Firefox:

const getKey = (website === "") ? null : website; // get "" returns an empty object! chrome.storage.local.get(getKey, function(result) {

@lapcat
Copy link

lapcat commented Oct 13, 2022

I see in the meeting notes that some people didn't like this behavior, but it's no different from standard JavaScript objects:

 > var testing = {"":"foo"};
 < undefined
 > testing;
 < {: "foo"}
 > testing[""];
 < "foo"
 > testing[""] = "bar";
 < "bar"
 > testing[""];
 < "bar"

@bershanskiy
Copy link
Member

I have a workaround for Safari that I don't need for Chrome or Firefox:

const getKey = (website === "") ? null : website; // get "" returns an empty object! chrome.storage.local.get(getKey, function(result) {

I actually created a PR for MDN to include this workaround. Not sure if it will be merged since this level of detail might be excessive for MDN.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
implemented: chrome Implemented in Chrome implemented: firefox Implemented in Firefox inconsistency Inconsistent behavior across browsers spec clarification Needs clarification when specified supportive: safari Supportive from Safari
Projects
None yet
Development

No branches or pull requests

5 participants