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

Slight bug for unsorted filtered store with defaultNewToStart set to true #45

Closed
mercmobily opened this issue Jul 29, 2014 · 2 comments
Closed

Comments

@mercmobily
Copy link

While trying out all possible test cases with stores, I found that a filtered store that has defaultNewToStart set as true will notify (in Observable) new elements as to be placed at the END of the dataset. This is even though the caching store has placed the element at the start (as required) -- and ditto for the DB server.

The problem is that in such a case the flow gets to line 290 of Observable: https://github.com/SitePen/dstore/blob/master/Observable.js#L290

candidateIndex = sampleArray.length;

The reason why it gets to this line is that since there is filtering, queryExecutor is defined and therefore it starts using it to work out where the element should be. At this point, only beforeId is used to place the element anywhere other than the end.

The solution is to change line 290 into:

candidateIndex = store.defaultNewToStart ? 0 : sampleArray.length;

However, I am not sure about something... see ADDENDUM.

ADDENDUM:
This rings a little strange to me) that if there is no filtering, the if in line 259 fails https://github.com/SitePen/dstore/blob/master/Observable.js#L259 :

if (queryExecutor) {

This means that stores without queries are basically treated the same as stores without any kind of queryEngine, since queryExecutor depends both on the presence of a queryEngine, and the presence of some kind of querying.

@mercmobily
Copy link
Author

About the ADDENDUM, at minimim this comment should be rewritten:

// we don't have a queryEngine, so we can't provide any information
// about where it was inserted or moved to. If it is an update, we leave
// its position alone. otherwise, we at least indicate a new object

Since this gets triggered not by the lack of queryEngine, but the lack of queryExecutor (that is, any kind of filtering/sorting/anything in the query).

However, I am looking at the code and wondering if this issue might point to some refactoring, both in the comments and the code itself.

@mercmobily
Copy link
Author

Glad I could help!

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

1 participant