Index creation behavior (defaulting and keyPath)

[brettz9]

The use of defaulting to unique: false in the following code is puzzling to me:

var indexKey;
for (indexKey in table.indexes) {
    var index = table.indexes[indexKey];
    try {
        store.index(indexKey);
    } catch (err) {
        store.createIndex(indexKey, index.key || indexKey, Object.keys(index).length ? index : { unique: false });
    }
}

Is there any advantage to explicitly declaring unique: false here rather than just always using index (including if empty)? It appears from the spec that the default is false anyways...

Also, would it not be clearer to check for index.keyPath || indexKey instead of index.key || indexKey?

[aaronpowell]

Is there any advantage to explicitly declaring unique: false here rather than just always using index (including if empty)? It appears from the spec that the default is false anyways...

Yes, the default is false but unless it was to explicitly set undefined for the last argument index is always going to represent something and that something might not be valid.

Also, would it not be clearer to check for index.keyPath || indexKey instead of index.key || indexKey ?

Then the property it looks for would have to be called keyPath not key on the schema object (which then could be a breaking change).

[brettz9]

Your existing Object.keys(index).length check:

store.createIndex(indexKey, index.key || indexKey, Object.keys(index).length ? index : { unique: false });

...will either:

1. throw an error because index is not an object
2. produce the same effect as {unique: false} if an empty object is supplied
3. cause index to return itself due to index being an object with properties

I therefore see no difference with:

store.createIndex(indexKey, index.key || indexKey, index);

...except that your approach is longer and will throw (whether upon an explicitly empty index or where createIndex might throw with a bad index type supplied anyways).

If you think that is important to throw rather than supply a bad index, then why not just use the following check?

store.createIndex(indexKey, index.key || indexKey, index && typeof index === 'object' ? index : {unique: false});

(If you didn't mind null or undefined, you could also drop the index && portion.)

That would seem clearer as to your intent. Not a big deal, just trying to confirm it was not an oversight.

As far as keyPath, why not deprecate key in favor of keyPath?

[aaronpowell]

except that your approach is longer and will throw (whether upon an explicitly empty index or where createIndex might throw with a bad index type supplied anyways).

Could you create a test case that demonstrates how you'd hit this problem?

[brettz9]

I'm not expecting that one would normally hit this problem without crafting a bad schema, but I'm just saying that if your purpose is to avoid, as you say, the "something" which "might not be valid", then your current approach isn't helping that much.

If a bad value is supplied as index to this portion:

Object.keys(index).length ? index : { unique: false }

...the following will occur with the Object.keys check:

• null or undefined, will throw, not default to {unique: false}, so your keys check doesn't help with these (and maybe even make it a tiny bit worse since, by throwing, people will not be able to request default index params in this manner).
• a string will return an array containing each character, so if a non-empty string is given, Object.keys will return an array with length and the string will be passed on unaltered, so your keys check doesn't help prevent this case of a string being supplied (unless it is an empty string).
• A boolean, number, empty string, empty array, or function will get a length of 0 and thus default to {unique: false} so your check will help prevent bad values being passed on in these cases.

Under your current code, an empty object will default to {unique:false} which is the expected default, though FWIW, {} would work just as well here.

Under your current code, a non-empty object will be passed on unaltered as expected (including, however, cases where bad or custom properties such as key are included on the object--which admittedly may not be an issue).

If your purpose is to circumvent all possible bad index values described in the list above, then the following code will do this more succinctly and will accurately avoid all of the bad value types above (and without throwing):

index && typeof index === 'object' ? index : {}

[brettz9]

This was handled in one of the PRs, so closing. Thanks for all the merges!