OfficeDev / office-js-helpers

[ARCHIVED] A collection of helpers to simplify development of Office Add-ins & Microsoft Teams Tabs
MIT License
126 stars 56 forks source link

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

Open yutao-huang opened 5 years ago

yutao-huang commented 5 years ago

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 commented 5 years ago

Adding the related issue: #126

yutao-huang commented 5 years ago

@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...)

Zlatkovsky commented 5 years ago

@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 commented 5 years ago

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.

yutao-huang commented 5 years ago

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