Skip to content
This repository has been archived by the owner on Mar 13, 2018. It is now read-only.

core-localstorage defaulting logic has bad null test #5

Closed
sjmiles opened this issue Sep 13, 2014 · 5 comments
Closed

core-localstorage defaulting logic has bad null test #5

sjmiles opened this issue Sep 13, 2014 · 5 comments
Labels

Comments

@sjmiles
Copy link
Contributor

sjmiles commented Sep 13, 2014

If there no value is in localstorage, core-localstorage tries not to create one unless it looks like the user has set a real value. However, the conditional is written such that a value of undefined does get saved.

https://github.com/Polymer/core-localstorage/blob/master/core-localstorage.html#L100

Should probably be:

if (this.value !== null && this.value !== undefined) {

@sjmiles sjmiles changed the title problem core-localstorage defaulting logic has bad null test core-localstorage defaulting logic has bad null test Sep 13, 2014
@sjmiles sjmiles added the p1 label Sep 13, 2014
@jmesserly
Copy link

fwiw, I think

if (this.value != null) {

is equivalent to

if (this.value !== null && this.value !== undefined) {

... I tend to use the former as it's a bit more concise.

@jmesserly
Copy link

(wanted to double check this, so found the relevant spec for == https://people.mozilla.org/~jorendorff/es6-draft.html#sec-abstract-equality-comparison)

@sjmiles
Copy link
Contributor Author

sjmiles commented Sep 14, 2014

You are right about that, and it's definitely more concise. OTOH, it's one of the most confusing bits of Javascript: null == undefined but null != false but Boolean(null) == false (so if (null) is false).

This is the main reason we prefer ===, and I'm inclined to be explicit in these cases. But it could go either way.

@sjmiles
Copy link
Contributor Author

sjmiles commented Sep 14, 2014

Depending on my mood, I'm waffling between brevity and clarity.

We could make a Polymer.defined method to try to split the difference (defined(value) is true if value != null). Eh, it's not worth the trouble.

@tomalec
Copy link

tomalec commented Sep 23, 2014

Thanks guys! You're fixing bugs faster than I'm able to isolate them from my app ;)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants