tinyplex / tinybase

The reactive data store for local‑first apps.
https://tinybase.org
MIT License
3.76k stars 80 forks source link

Unexpected behavior with ids parsed on local storage. #27

Closed kastriotkastrati closed 2 years ago

kastriotkastrati commented 2 years ago

Is your feature request related to a problem? Please describe. A clear and concise description of what the problem is. Ex. I'm always frustrated when [...]

Assume you set a table like this:

store.setTable('tableName', { parseInt('123'): { 'attr': 1, ...otherAttrs }})

This is wrong because rowId should not be a number but stick with me.

If later you set a cell in this table and rowId like.

store.setCell('tableName', parseInt('123'), 'attr', 2);

This will override the localStorage instance of this rowId to only { 'attr': 2 }. There's no other attributes that we initially set. I'm assuming this happens because when we set the table with a rowId of type number, internally this number is casted into a string. When on setCell we offer the same rowId of type number with which we set the table, the rowId is not a string anymore, thus the instance cannot be found and an empty object is returned, which we then populate with the value of setCell. And ultimately, when the save operations happens, the rowId of type number is again casted as string which then overrides the localStorage instance

Describe the solution you'd like A clear and concise description of what you want to happen.

Just a warning would be nice whenever we're setting rowIds as number, or in a more consistent fashion with the error handling of tinybase until now, we could have it just silently fail the operation of setting a cell if the rowId provided is a number.

Describe alternatives you've considered A clear and concise description of any alternative solutions or features you've considered.

Additional context Add any other context or screenshots about the feature request here.

jamesgpearce commented 2 years ago

Sorry I was off the grid for a few days. I’ll take a look at this and propose a few solutions. Thanks for your patience and being a TinyBase supporter! :)

jamesgpearce commented 2 years ago

@kastriotkastrati take a look at the test cases in https://github.com/tinyplex/tinybase/blob/43a56d849db658980932b20db11b62ac174b720d/test/unit/foolishness.test.tsx#L410-L527 - these failed without the other changes in the diff. Basically everything gets coerced to a string at the API level, so no numeric keys should ever exist or be queried in the underlying storage structure.

jamesgpearce commented 2 years ago

Please update to package v1.3.6 and confirm the issue is resolved! Thanks.

kastriotkastrati commented 2 years ago

@jamesgpearce Yep, the issue is solved. Thank you:) https://stackblitz.com/edit/react-ts-bkf8jn?file=App.tsx