Skip to content
This repository has been archived by the owner on May 3, 2022. It is now read-only.

Should keys be restricted to strings? Should values? #2

Closed
domenic opened this issue Feb 2, 2018 · 24 comments
Closed

Should keys be restricted to strings? Should values? #2

domenic opened this issue Feb 2, 2018 · 24 comments
Labels
api A proposal for changing the API surface

Comments

@domenic
Copy link
Collaborator

domenic commented Feb 2, 2018

Local storage restricts keys and values to strings. Should async local storage do so?

I think we should not restrict values. localForage does not, and specifically touts it as a feature. In general this causes lots of JSON-serialization round-tripping, with the attendant pitfalls.

I'm less sure about the keys. localForage casts any given value to a string key. Maybe we should stick with that.

On the other hand, it's more code to disallow arbitrary keys...

@domenic domenic added the api A proposal for changing the API surface label Feb 12, 2018
@ebidel
Copy link

ebidel commented Feb 12, 2018

Strong +1 on not restricting to strings. I've never had a case where storing only strings has been helpful. In fact, it's typically the opposite like you say. Lots of bugs and unnecessary overhead.

localStorage.setItem('foo', 10);
localStorage.getItem('foo') + 10 // oops!

localStorage.setItem('bar', {val: 10});
localStorage.getItem('bar').val // nice try!

It would be interesting to look at httparhive data to see the "% of localstorage usage that's serialized json". But @rviscomi tells me they only look at size in the crawl, not actual content.

@domenic
Copy link
Collaborator Author

domenic commented Feb 12, 2018

@ebidel It sounds like you are mostly talking about values. Do you have an opinion on non-string keys as well?

@ebidel
Copy link

ebidel commented Feb 12, 2018

Yea, sorry. Values! Haven't thought about keys too much, but it might be useful to make it similar to map.

@jakearchibald
Copy link

I'm sure someone will be caught out by 10 and "10" being separate keys with different values. But, I think the benefits outweigh that.

This was referenced Feb 26, 2018
@domenic
Copy link
Collaborator Author

domenic commented Feb 28, 2018

To be more concrete, now that I've learned a bit more about the subject, the possible keys are:

  • numbers
  • strings
  • Date instances
  • ArrayBuffers
  • DataViews and typed arrays
  • arrays of the above

@SanderElias
Copy link

SanderElias commented May 30, 2018

I would opt for key's forced to be strings.
For one, it makes it way easier to serialize to/from json. As this is a persistent storage, there will be a need to serialize it rather sooner as later. It makes a lot of sense to serialise to json. The alternative would be a custom serialization (and deserialization) with would add more code as the restricting to strings for keys would cost.

For a normal map, alternatives to string make sense, but for a map that is persistent, it will only complicate things, in the long run, I'm afraid.

For values, anything should be allowed, except for undefined/null. storing those should be the equivalent of removing the property in question.

@jakearchibald
Copy link

@SanderElias are you also arguing against values that aren't JSON-serialisable, such as dates?

@SanderElias
Copy link

SanderElias commented May 30, 2018

@jakearchibald dates are one-way JSON-serialisable ;)
Yeah, I think it would make a valid decision to reject any value that is not JSON-serialisable.

so restring the types to string|boolean|number|object|array were the array an object only allows for 'string|boolean|number` will make a lot of sense.

Perhaps allow dates in there too, but make a note that on deserialization you will get an iso string instead of a date. (that also happens on normal JSON.parse, so it's not an unprecedented exception!)

Note the absence of undefined|null. It makes no sense to store those. see the discussion in #3

@SanderElias
Copy link

As an extra explainer:

I think if we want to allow for keys and/or values that don't serialize to JSON well, the library needs to expose a way to serialize/deserialize to a sting|buffer. Developers need to able to sync/share their storage between different browsers/session/systems. (perhaps even between different apps...)

As this proposal will lift a lot of issues that are in current local storage, this becomes even more important.

@jakearchibald
Copy link

I don't think we should apply restrictions on either keys or values based on JSON. For instance, being able to store blobs is really useful, and having to convert to/from string representations just to keep JSON happy (a restriction that may not apply to you) sounds like a lot of main-thread work we could do without.

If developers want to apply a more-restrictive schema in particular use-cases, a little wrapper library can do that.

@SanderElias
Copy link

@jakearchibald I agree with you that it would be very useful to have those other types too.

However, you seem to skip over the part where I explain about the whole serializability.
When there is more support as just JSON allowed types. the library really should come up with a way
to make it easy to "backup"/"restore" the data set in there.

In modern applications, we can not get away anymore with data that is restricted to a single browser/device. When you are building something that can store more as just some trivial stuff, you become responsible for that use-case too. In my eye's this is the biggest flaw in IndexDB, and hindering acceptance.
When building something new, you should not repeat mistakes made in the past!

You can "backup/restore" localStorage pretty easy. This library should have a similar pattern for that.
What I said was that JSON makes it unneeded to add extra code to support this serialization/deserialization!

@domenic
Copy link
Collaborator Author

domenic commented May 30, 2018

We're definitely not going to restrict this API to be JSON-serializable. You can choose, as a web developer, to only put JSON-serializable types into it. But we're not going to make that decision for everyone at the API design level.

@SanderElias
Copy link

@domenic I'm fine with not limiting to JSON.

So, should I open up a new issue about the serialization point, or are we going to discuss that in here?

@domenic
Copy link
Collaborator Author

domenic commented May 30, 2018

You can if you want. Keep in mind we're not really going to add new capabilities beyond what local storage/IndexedDB already has, so you should just use the same techniques you use for those technologies when using async local storage. If you'd like a dedicated issue to guide you through how to do that though, we can probably help.

@SanderElias
Copy link

@domenic Well, LocalStorage really doesn't need anything special, so it depends on the definition off what you mean by 'has' ;)

Serializing localStrorage for backup/sync:

const backup = JSON.stringify(localStorage)

//restore:
JSON.parse(backup).entries().forEach(pair => localStorage.set(pair[0],pair[1]))

Something simple like this should work for async-local-storage too when it's aimed to be a better localStorage

@jakearchibald
Copy link

Look at some of the hoops developers have to jump through to efficiently store things like binary data in localstorage https://labs.ft.com/2012/06/text-re-encoding-for-optimising-storage-capacity-in-the-browser/.

The restrictions you're talking about, that may benefit you, would be a burden to many others. Applying your particular requirements in user-land is pretty trivial. But as you can see from the article above, working around the restrictions you want to apply to the platform isn't trivial at all.

@SanderElias
Copy link

@jakearchibald I do NOT want to restrict anything. I would love to have a localStorage replacement that can store all variable types!

However, As it is replacing localStorage, it also should have an easy way to serialize. If there is no easy way to do that, you are locking the data that is stored in there in a single browsers instance.

All I am saying is that async-local-storage should provide the same ease for serialization.

input { Storage } form ...
// backup can be a string, or a buffer.
const backup = Storage.serialize()

// restore
Storage.deserialize(backup)

In my eye's this is a mandatory provision, if you want to replace localStorage

@domenic
Copy link
Collaborator Author

domenic commented Jun 1, 2018

Almost the same code will work for async local storage as for local storage. Just replace JSON.stringify(localStorage) with JSON.stringify(await storage.entries()) and add some awaits to your set() calls.

@SanderElias
Copy link

@domenic Is that true? what happens if there is a typed array, and arrayBuffer or a databuffer?
Not trolling, but a real concern.

@domenic
Copy link
Collaborator Author

domenic commented Jun 1, 2018

Don't use those! Async local storage is just as usable as normal local storage for your very specific purposes, if you keep using it in the same way you're using local storage (i.e. if you don't use types that don't serialize to JSON).

@SanderElias
Copy link

SanderElias commented Jun 1, 2018

So you are saying there will not be a simple way to sync an async-local-storage between browsers/devices? Not without knowing exactly what its contents is on beforehand?

@jakearchibald
Copy link

@SanderElias

I would opt for key's forced to be strings.

Yeah, I think it would make a valid decision to reject any value that is not JSON-serialisable.

so restring the types to string|boolean|number|object|array were the array an object only allows for 'string|boolean|number` will make a lot of sense.

I do NOT want to restrict anything.

@SanderElias
Copy link

SanderElias commented Jun 1, 2018

@jakearchibald :) All I wanted is for the storage to be serializable. As in the first message Dominic mentioned the amount of code as being a limiting factor, I thought along those lines and made the statements to ease that. If it would be jsonable, no extra code would be needed for serialization.

to quote me:

For one, it makes it way easier to serialize to/from json. As this is a persistent storage, there will be a need to serialize it rather sooner as later. It makes a lot of sense to serialise to json. The alternative would be a custom serialization (and deserialization) with would add more code as the restricting to strings for keys would cost.

I don't want to restrict anything, so that includes not restricting my data to live on 1 browser/device.

@jakearchibald
Copy link

@SanderElias I think what you're looking for is whatwg/html#3517

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api A proposal for changing the API surface
Projects
None yet
Development

No branches or pull requests

4 participants