search

jakearchibald / idb

IndexedDB, but with promises
https://www.npmjs.com/package/idb
ISC License
6.2k stars 348 forks source link

Incorrect type of cursor value #286

Open vitonsky opened 1 year ago

vitonsky commented 1 year ago

I have an IDB scheme https://github.com/translate-tools/linguist/blob/4da90de9e406442ab78df591bad04d7e6b93cdf3/src/requests/backend/translations/idb/schema/v2.ts#L8-L16

Here i use cursor value https://github.com/translate-tools/linguist/blob/4da90de9e406442ab78df591bad04d7e6b93cdf3/src/requests/backend/translations/data.ts#L135

And i found that type of cursor value incorrect, when my test failed. The type is not contains a "keyPath" that i defined here https://github.com/translate-tools/linguist/blob/4da90de9e406442ab78df591bad04d7e6b93cdf3/src/requests/backend/translations/idb/schema/v2.ts#L59-L62

So cursor value contains not only properties of type ITranslationEntry but also property id that is auto incremented number.

It's my temporary fix for test https://github.com/translate-tools/linguist/blob/4da90de9e406442ab78df591bad04d7e6b93cdf3/src/requests/backend/translations/data.test.ts#L40-L42 and i have to refactor a lot of code to remove this unnecessary property.

Let's improve types. We can add a keyPath to a IDB scheme and merge it to a cursor value type and in another places when we get values from IDB

jakearchibald commented 1 year ago

To make it easier for me to debug this, can you reproduce the issue in the Typescript playground? Here's a demo that shows a cursor with a correct type.

vitonsky commented 1 year ago

@jakearchibald yes, demo with broken types

vitonsky commented 1 year ago

We have to improve type for createObjectStore, to require exists property for keyPath. We must consider case with a composite keyPath

jakearchibald commented 1 year ago

It looks like you've just missed part of the value definition? This works.

vitonsky commented 1 year ago

@jakearchibald you're right, we use typescript exactly to avoid data misses. Types for createObjectStore must make impossible a missed property and generate compile time error for such cases

jakearchibald commented 1 year ago

I'll accept a PR for this if it works, but keyPath is pretty complex.

vitonsky commented 1 year ago

@jakearchibald agree about complexity, maybe i will try to fix when will have time. Let's mark this as bug and needs help

dumbmatter commented 1 year ago

I think my PR #252 solves this issue - add and put accept an object with or without the auto incrementing keyPath being present, but any object returned from the database has the keyPath value defined. Example

jakearchibald commented 1 year ago

I'll find some time to take a close look at that. Sorry I haven't so far.