feat(store-sync): sync to zustand #1843
Conversation
🦋 Changeset detectedLatest commit: fbda87f The changes in this PR will be included in the next version bump. This PR includes changesets to release 30 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| tables: {}, | ||
| rawRecords: {}, | ||
| records: {}, | ||
| getRecords: <table extends Table>(table: table): TableRecords<table> => { |
There was a problem hiding this comment.
ok maybe i'm slowly getting used to table extends Table
| const records = Object.values(get().records); | ||
| const expectedKeyTuple = encodeKey(flattenSchema(table.keySchema), key); | ||
| const record = records.find( | ||
| (record) => | ||
| record.table.tableId === table.tableId && concatHex([...record.keyTuple]) === concatHex(expectedKeyTuple) | ||
| ); | ||
| return record as TableRecord<table> | undefined; |
There was a problem hiding this comment.
i'm a little bit scared of performance of finding records via linear search if the table has a lot of entries (maybe not the use case of the zustand store? but still).
Couldn't we make the id just the concatenated tableId + key to be able to look it up immediately? What are we solving for by adding the address? (which i think is the reason why we can't just look up the record by id here correct?) If we had records from multiple worlds in here, then I think the current approach would just return the first record independent of the address part of its id?
There was a problem hiding this comment.
I considered this, but all of the existing sync adapters support an optional address (i.e. either one world or all worlds), so this was here to keep compatibility/predictability. We could make address a required param for syncToZustand and then get rid of the address as part of the ID/object key and that would make look ups faster.
Now that I am writing this, I am realizing that even though we key on address, we don't actually return it anywhere or allow filtering on it in getRecord, getRecords, etc. So maybe it is best if we make this a required param for now, and figure out how to make it optional/multi-world later.
Another thing on my mind is potentially keeping the top-level keys (records, tables, etc.) "out of view" of downstream consumers and forcing them to use things like getRecord and getRecords so we can modify the underlying state representation.
There was a problem hiding this comment.
comments are not blocking but i'm curious about the id/getRecord approach
see changeset