Add support for multi index querying #90
Conversation
There was a problem hiding this comment.
Thanks for the PR!
Please run prettier (yarn run-prettier) on all files, to ensure the formatting is right. Also, please rebase the branch on upstream master - it seems I have a faulty GH action setup, so the tests are not running on the PR - that should hopefully be fixed after the rebase.
I left some review comments 👍
| @@ -26,6 +26,7 @@ | |||
| "release": "release-it" | |||
| }, | |||
| "dependencies": { | |||
| "@babel/plugin-proposal-class-properties": "^7.8.3", | |||
There was a problem hiding this comment.
Please remove this, shouldn't be necessary.
There was a problem hiding this comment.
I was having trouble getting it to link inside of my project.
| (query, [queryKey, queryValue], i) => { | ||
| let index = indexMap[queryKey]; | ||
|
|
||
| assert(index, `You are attempting to query ${queryKey} which is not a valid index for ${type}`); |
There was a problem hiding this comment.
The order of arguments is wrong here - should be the message first, then the check :) so
assert(`...`, !!index)]|
|
||
| return promise; | ||
| return Object.entries(query) | ||
| .reduce( |
There was a problem hiding this comment.
Please avoid using reduce, it is hard to read and follow.
There was a problem hiding this comment.
@mydea It's your project so I'm happy to change it :)
But!
I don't necessarily agree. Reduce is self-documenting. When I see reduce I know that you are accumulating a collection into something else, so without ever reading the code I have some context to start with. Whereas when I see a for loop iterating over some collection, it could be doing any number of things, with all sorts of side effects.
Another benefit of using reduction is that you have a pure function that accumulates the query, in general, I find pure functions are easier to reason about as they only have inputs and outputs, and no side effects, which makes writing tests for them a bit less onerous.
I notice you do a lot of forEach loops where you append to a list, in those cases why not use map/filter? When I see map, I know that I'm transforming a single list into another list of the same size, but potentially different type.
Again, it's your project, and I know people get tripped up on reduce, and some higher-order functions in general, just some food for thought :)
There was a problem hiding this comment.
Yes, there are for sure some places where refactoring to map() makes sense :)
Regarding reduce(), I try to only use it for very short things, e.g. when summing a number. For more complex things (like this), I think it obscures a bit what is going on.
I think it is also OK to leave it with reduce, and add some comments about what is going on and why. Especially as the Dexie/IndexedDb syntax can be a bit tricky, so it is not that obvious why certain things happen :)
|
|
||
| return query.and(predicate); | ||
| } | ||
| }, db[type]); | ||
| } |
There was a problem hiding this comment.
We should probably add distinct() before we return it, if at least one is a multi entry index (see: https://dexie.org/docs/MultiEntry-Index)
There was a problem hiding this comment.
Yeah, that makes sense, I was also thinking we should maybe sort the indexes by multi beforehand, from a performance perspective I would expect that having IndexedDb do at least one #anyOf query would get you some benefit overdoing the set intersection in memory.
Spike implementation of querying multi index.