Discussion on: Simple Javascript Modules - Local Storage Module

[Mihail Malo]

I'm sorry, but this code makes no sense and is frankly confusing and dangerous.
It doesn't really add any convenience over just using window.localStorage directly, besides the JSON conversion.
This can be achieved with just

export class JSONStorage {
  constructor(storage) {
    if (!storage || typeof storage !== "object")
      throw new Error(`Expected a Storage object, got ${storage}`)
    this.storage = storage
  }
  set(k, v) {
    const str = JSON.stringify(v)
    if (typeof str === "undefined")
      return this.storage.removeItem(k)
    this.storage.setItem(k, v)
  }
  get(k) {
    const str = this.storage.getItem(k)
    // we return `undefined` instead of `null`, so that we can differentiate between the cases when the key doesn't exist vs when the value is a JSON-serialized `"null"`
    if (str === null) return
    return JSON.parse(str)
  }
  remove(k) {
    this.storage.removeItem(k)
  }
}

you can try it as follows

const a = new JSONStorage(window.localStorage)
const b = new JSONStorage(window.sessionStorage)
a.set("1", 1)
b.set("1", 2)
// notice that keys are coerced to string automatically, and we get back deserialized numbers, not strings
a.get(1) // 1
b.get(1) // 2

Instead, the provided code:

1. checks if localStorage is supported on every action, instead of just once.
2. calls .getItem(key) twice, which might be the most expensive operation here, short of setItem
3. similarly, calls getItem before removeItem
4. since !"" is true, empty values (if created outside this interface) will never be removed!
5. uses a mutable field data on the singleton object instead of a local variable. This data never needs to survive beyond a function call, and is being reused by all accesses to Storage.
6. constructs a {key, data} object with the function's arguments, which will never be used and doesn't offer any ergonomic benefit.
7. declares a function to just set null to this.data.
8. on most error conditions (such as passing a number as key, because numbers don't have .length), randomly returns the contents of this.data, which will always be null because every method, even remove that doesn't use it first calls clearData. This is confusing, since the standard getItem returns null for missing key, and "null" is a valid JSON string.
9. makes an object instance named Storage to then just export it as default, instead of exporting the individual functions, avoiding a layer of indentation.

PS: Keep in mind, my code still doesn't prevent you from calling, say, b.set() which would create a undefined => undefined entry, which would cause a JSON SyntaxError when calling b.get(), nor does it exhaustively check for the storage object passed to the constructor having the necessary methods.

Edit: The former is quite serious, so I fixed it now. This now removes the key, so setting a value of undefined and then getting it will, in fact, return undefined

[Sooraj (PS)]

Thank you for the informative points :)