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

Add support for multi index querying #90

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
/npm-debug.log*
/testem.log
/yarn-error.log
.idea

# ember-try
/.node_modules.ember-try/
Expand Down
57 changes: 34 additions & 23 deletions addon/services/indexed-db.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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!');
Expand Down Expand Up @@ -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!
Expand All @@ -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(
Copy link
Owner

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.

Copy link
Author

@joegaudet joegaudet Mar 24, 2020

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 :)

Copy link
Owner

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 :)

(query, [queryKey, queryValue], i) => {
let index = indexMap[queryKey];

assert(index, `You are attempting to query ${queryKey} which is not a valid index for ${type}`);
Copy link
Owner

Choose a reason for hiding this comment

The 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)]

Copy link
Author

Choose a reason for hiding this comment

The 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]);
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Author

Choose a reason for hiding this comment

The 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 #anyOf query would get you some benefit overdoing the set intersection in memory.


_mapItem(type, item) {
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
"release": "release-it"
},
"dependencies": {
"@babel/plugin-proposal-class-properties": "^7.8.3",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this, shouldn't be necessary.

Copy link
Author

Choose a reason for hiding this comment

The 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",
Expand Down