-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add support for multi index querying #90
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,7 @@ | |
/npm-debug.log* | ||
/testem.log | ||
/yarn-error.log | ||
.idea | ||
|
||
# ember-try | ||
/.node_modules.ember-try/ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,14 +3,15 @@ import { set, get } from '@ember/object'; | |
import { Promise } from 'rsvp'; | ||
import { later } from '@ember/runloop'; | ||
import { typeOf as getTypeOf } from '@ember/utils'; | ||
import { A as array } from '@ember/array'; | ||
import { A as array, isArray } from '@ember/array'; | ||
import { | ||
registerWaiter, | ||
unregisterWaiter, | ||
} from 'ember-indexeddb/utils/test-waiter'; | ||
import { task, timeout } from 'ember-concurrency'; | ||
import { log } from 'ember-indexeddb/utils/log'; | ||
import Dexie from 'dexie'; | ||
import { assert } from '@ember/debug'; | ||
|
||
/** | ||
* This service allows interacting with an IndexedDB database. | ||
|
@@ -477,7 +478,7 @@ export default class IndexedDbService extends Service { | |
} | ||
|
||
@task(function* (config) { | ||
let { databaseName, version, stores, data } = config; | ||
let {databaseName, version, stores, data} = config; | ||
|
||
log('===================================='); | ||
log('Importing database dump!'); | ||
|
@@ -577,15 +578,15 @@ export default class IndexedDbService extends Service { | |
} | ||
} | ||
|
||
// Only one, then do a simple where | ||
if (keys.length === 1) { | ||
let key = keys[0]; | ||
return db[type].where(key).equals(query[key]); | ||
} | ||
|
||
// Order of query params is important! | ||
let { schema } = db[type]; | ||
let { indexes } = schema; | ||
let {schema} = db[type]; | ||
let {indexes} = schema; | ||
let indexMap = indexes.reduce((acc, index) => { | ||
let keyPath = get(index, 'keyPath'); | ||
acc[keyPath] = index; | ||
|
||
return acc; | ||
}, {}); | ||
|
||
// try to find a fitting multi index | ||
// only if the client supports compound indices! | ||
|
@@ -609,26 +610,36 @@ export default class IndexedDbService extends Service { | |
// If a multi index is found, use it | ||
if (multiIndex) { | ||
let keyPath = get(multiIndex, 'keyPath'); | ||
let compareValues = array(); | ||
keyPath.forEach((key) => { | ||
compareValues.push(query[key]); | ||
}); | ||
let compareValues = keyPath.map((key) => query[key]); | ||
|
||
let keyName = get(multiIndex, 'name'); | ||
return db[type].where(keyName).equals(compareValues); | ||
} | ||
} | ||
|
||
// Else, filter manually | ||
Object.keys(query).forEach((i) => { | ||
if (!promise) { | ||
promise = db[type].where(i).equals(query[i]); | ||
} else { | ||
promise = promise.and((item) => get(item, i) === query[i]); | ||
} | ||
}); | ||
|
||
return promise; | ||
return Object.entries(query) | ||
.reduce( | ||
(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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The order of arguments is wrong here - should be the message first, then the check :) so assert(`...`, !!index)] There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Whoops! |
||
|
||
let isMulti = index.multi && isArray(queryValue); | ||
let first = i === 0; | ||
|
||
if (first) { | ||
return isMulti | ||
? query.where(queryKey).anyOf(...queryValue) | ||
: query.where(queryKey).equals(queryValue); | ||
} else { | ||
let predicate = isMulti | ||
? (item) => get(item, queryKey).any(_ => queryValue.includes(_)) | ||
: (item) => get(item, queryKey) === queryValue; | ||
|
||
return query.and(predicate); | ||
} | ||
}, db[type]); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should probably add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
||
_mapItem(type, item) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,6 +26,7 @@ | |
"release": "release-it" | ||
}, | ||
"dependencies": { | ||
"@babel/plugin-proposal-class-properties": "^7.8.3", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please remove this, shouldn't be necessary. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was having trouble getting it to link inside of my project. |
||
"dexie": "^2.0.4", | ||
"ember-auto-import": "^1.5.3", | ||
"ember-cli-babel": "^7.18.0", | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please avoid using reduce, it is hard to read and follow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)