office-js-helpers icon indicating copy to clipboard operation
office-js-helpers copied to clipboard

In the implementation of Storage.has(key), we should check against null instead of undefined to see if a key exists in the storage.

Open yutao-huang opened this issue 6 years ago • 5 comments

According to the W3C standard (https://www.w3.org/TR/webstorage/#dom-storage-getitem):

... If the given key does not exist in the list associated with the object then this method must return null.

In the existing code, this.get(key) !== undefined would always be true even when the key doesn't exist. In this case, this.get(key) actually returns null.

This incorrect behavior is also impacting things like authenticator.endpoints.has(...), which always mistakenly returns true for an actually not registered endpoint.

yutao-huang avatar Feb 05 '19 18:02 yutao-huang

Adding the related issue: #126

yutao-huang avatar Feb 05 '19 18:02 yutao-huang

@Zlatkovsky thanks for the approval!

I've verified the behavior of localStorage.getItem on Chrome/Edge/FireFox/Safari/IE and all returns null for non-existent keys. Wondering what could be the situation that would make it return undefined. (I did not check for older versions, though. I assume the behaviors of the browsers should not have changed...)

yutao-huang avatar Feb 06 '19 02:02 yutao-huang

@yutao-huang , it comes down to implementation details. For example, depending on whether the underlying implementation uses localStorage[xyz] syntax or localStorage.getItem(xyz) syntax, you get two different results!

image

I expect that's how the bug found its way here in the first place -- that Bhargav had originally had one implementation, then changed it out for another, and suddenly an older assumption about undefined was no longer true.

While null and undefined are semantically different, I would argue that they are mostly the same for purposes of this class. So I think it's better to check for both rather than have it only do one, and then risk an internal-implementation change breaking the outer contract.

Zlatkovsky avatar Feb 06 '19 03:02 Zlatkovsky

As for my approval -- note that you still will need someone else like @sumurthy to actually merge in the changes. I don't have any admin rights on this repo, so I don't know if my "approval" means much per se.

Zlatkovsky avatar Feb 06 '19 03:02 Zlatkovsky

@Zlatkovsky , gotcha, that makes sense. I added the check for safety. I'll ping @sumurthy to get some help. Thanks!

yutao-huang avatar Feb 07 '19 05:02 yutao-huang