fix: sort function will return numeric value

[esroyo]

Two points here:

• According to the standard the sort function should return a value > than 0 or < than 0 or 0. Not a boolean, which is the current result of the sort function.
• When selecting between the firstValue or the secondValue, I think undefined should never win. The rationale is: If we have some undefineds, we can deduce they are from unintended data loss (browser or user deleting the data from some storages). If we had used ImmortalDB.remove(...) then all storages would return undefined. Thus in the later case, there will be no doubt all would return undefined. If some non-undefined value survives, then those values should win in this comparison.

[gruns]

According to the standard the sort function should return a value > than 0 or < than 0 or 0. Not a boolean, which is the current result of the sort function.

counted.sort((a, b) => b[1] - a[1])

lgtm. great eyes!

When selecting between the firstValue or the secondValue, I think undefined should never win.

what about when the user overwrites a non-undefined value with undefined?

in this case, undefined should be returned over the old, overwritten non-undefined value

[esroyo]

Your point is interesting @gruns, I admit I didn't deeply though about that.

I mainly see two issues around undefined:

1 - The first one is that the underlying storages have different behaviors. Cookies and WebStorage on one hand can't possibly store an undefined value. They only store strings, so in that case there would be an implicit cast to string, and you would store "undefined". And when retrieving, get back "undefined", not the original undefined. IndexedDB on the other hand is not limited to strings, and you could store and retrieve a real undefined. Is ImmortalDB currently handling that inconsistency? Or we would get a count of ["undefined", 2], [undefined, 1] ?

2 - The second thing is that idb-keyval doesn't seem to provide a way to tell apart an undefined explicitly set as a value, from a non-existing key, which would return undefined also. That ambiguity is in idb-keyval itself, according to their readme [0].

May be the simplest approach is to make the user responsible of the inconsistencies, by clearly stating that keys and values should always be strings. Otherways keeping consistency becomes a problem. Imagine I try ImmortalDB.set('obj', {}), on WebStorage I will store "[object Object]" and on idb an object. If you don't support that, then why would you support ImmortalDB.set('obj', undefined) ?

If there is only support for string values, then we could assume that a literal undefined exactly means absence of registry. At least the storages are consistent in that, all return undefined for non-existing keys.

Also JSON itself does not support undefined, assumes it is the same as absence of value:

JSON.stringify({ one: undefined, two: null });
// "{\"two\":null}"

That is unrelated, but makes me think there is no good in trying to store that.

Thanks for your time, really appreciated.

[0] https://www.npmjs.com/package/idb-keyval

[gruns]

May be the simplest approach is to make the user responsible of the inconsistencies, by clearly stating that keys and values should always be strings.

yep. this is exactly what immortaldb does: keys and values must be strings to be compatible with the lowest common denominator storage engine beneath

it's up to the user of immortaldb to serialize/deserialize to/from strings appropriately

it states this in the docs:

`key` and `value` must be
[DOMStrings](https://developer.mozilla.org/en-US/docs/Web/API/DOMString).

if you set a key to undefined with immortaldb, the string literal 'undefined' is actually stored instead. again, it's up to the user to serialized/deserialize to/from strings appropriately

if this becomes a large issue, perhaps we'll add an interface on top which serializes/deserializes for the user automatically. it hasn't been an issue so far

[esroyo]

That is excellent @gruns , but then I don't fully understand your initial comment:

what about when the user overwrites a non-undefined value with undefined? in this case, undefined should be returned over the old, overwritten non-undefined value

A user can't set a value to undefined, right? (would always store "undefined"). So in this situation:

     if (firstCount >= secondCount && firstValue !== undefined) {

When firstValue === undefined that always means value missing. I assume that is the "bad thing".

May be your concern is about an unsuccessful remove() operation that leaves behind rogue/stale values?

[gruns]

A user can't set a value to undefined, right?

right. ignore my first comment

undefined means the key hasn't been set() or was remove()d

[esroyo]

great!

Then the only concern left is the case when undefined is the firstValue legitimately. When is that? I guess just on unsuccessful remove() operations that leave stale values behind.

I wonder how is that likely to happen. Or if that case is of bigger relevance.

To me It is not as important as recovering the values deleted by external forces. In my use case, I have 5 different storages, and most of them are delete either by the user or the browser. Usually I get 3 or 4 undefined VS 1 or 2 surviving values. firstCount always wins and I lose the value.

May be, for the case of legit remove() failure we could reject in the ImmortalDB.set() operation? That way the developer could have the chance to handle the failure and the ambiguous state would be their responsibility.

[gruns]

hey @esroyo!

did you fix/resolve this issue in a local fork of immortaldb? or is this issue no longer relevant to your usage and thus fine to close? 🙂

[esroyo]

hey @esroyo!

did you fix/resolve this issue in a local fork of immortaldb? or is this issue no longer relevant to your usage and thus fine to close? slightly_smiling_face

Hi @gruns :) I still think the issue is relevant. We started a fork to make sure nullish values were not taken into account whenever a non nullish value existed, fast. But ended up with lots of modifications, style changes, and other new needs, that provably are not be of general interest. At this point, code has diverged substantially. Having been so long since the issue started, I thought It was best to close it.

We are grateful of your work, though. I could look into how to bring in just the relevant bits, if you want.