Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect type of cursor value #286

Open
vitonsky opened this issue Nov 9, 2022 · 9 comments
Open

Incorrect type of cursor value #286

vitonsky opened this issue Nov 9, 2022 · 9 comments

Comments

@vitonsky
Copy link

vitonsky commented Nov 9, 2022

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
Copy link
Owner

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
Copy link
Author

@vitonsky
Copy link
Author

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

@jakearchibald
Copy link
Owner

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

@vitonsky
Copy link
Author

@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
Copy link
Owner

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

@vitonsky
Copy link
Author

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

@dumbmatter
Copy link

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
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants