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

adds an 'order-cache' to retrieve models in the order of the given payload #38

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

marcogrueter
Copy link

solves Ordering of results #34

only used it in 1 project so far (integrated via npm), so further testing might be needed.
But the changes are not that complicated...

Probably won't get pulled, but maybe help someone having the same problem with ordered data.

@beauby
Copy link
Owner

beauby commented Apr 18, 2017

Hi @marcogrueter – thanks for the PR. As mentioned somewhere else, I do not have time to commit to maintaining this project in the foreseeable future (adding new features or reviewing PRs), but I would make an exception for this issue because it's actually fixing a bug.

So I'll merge this if the scope changes from "keeping the whole store ordered" to "keeping the return value of sync ordered", as there is no clear ordering of the store itself once multiple payloads have been merged into it. Thoughts?

@marcogrueter
Copy link
Author

Hi @beauby, thank you very much for your feedback and thanks for taking time to review the PR.

I'm not quite sure if I got what you want mean.

Currently, the only ordering of data occurs when using the "findAll" method. Changes in the destroy and initModel methods are only to keep track of the position of an entry for a specific type. The graph itself is still indexed by id and therefore in whatever order the client (browser) seems fit.

Not ordering the data returned from the sync method is a dumb omission on my part (I actually never used the return of the sync methods, I'm always using find/findAll to get the data), that's certainly something that needs to be done.
Is that all that you meant?

Thanks again for your time and sorry for needing additional feedback.

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

Successfully merging this pull request may close these issues.

None yet

2 participants