search

genkgo / ember-localforage-adapter

Offline usage for Ember Data, based on localstorage adapter, but now uses Mozilla's localforage as data source
Other
133 stars 26 forks source link

Implement `queryRecord` (introduced by Ember 1.13) #36

Closed sebweaver closed 9 years ago

sebweaver commented 9 years ago

Hello (again),

Not really a compatibility issue, but I thought suggesting an implementation for the new queryRecord could help. For now, this implementation returns the first record matching the given query (instead of an array) or reject if not found (instead of an empty array).

Happy to discuss that further if you have another idea of implementation.

I added the related tests.

Regards,

Sébastien

frederikbosch commented 9 years ago

@sebweaver Let me read upon this. Will come back to it.

BTW: Same counts for pushPayload at the serializer level. I just did some tests with the REST pushPayload method, and I think there are just a few tricks needed to get it working.

frederikbosch commented 9 years ago

Thanks again @sebweaver!! I fixed one issue in #37. In your PR, the result of the last test was used instead of asserting that all tests should pass. I also made some readibility improvements. Nonetheless, thanks for contributing: your efforts are much appreciated! This will be tagged tomorrow.

sebweaver commented 9 years ago

You're welcome.

Good news for the issue you've fixed. It didn't jump out at me when I added the new argument. Perhaps we should add a test on that case, because the existing tests don't cover it.

Actually and more generally, my intention and my effort were mainly focused on making the package compatible with 1.13, not on optimizing, restyling or revamping the working (or not) pieces of code. Mixing too many objectives per PR could lead to introduce new issues much more difficult to track: will the issues be related to the migration or due to the rewriting? One objective at a time also keep the diff more concise and readable.

For example, here is a few ideas I had, during the upgrade process, for further dedicated PRs:

It would be my pleasure to take part in them!

I also have an idea about a new implementation to discuss with you, but shush... it's too soon, I'm too new on the projet... ;-)

frederikbosch commented 9 years ago

@sebweaver Made start on the clean code, by reformatting whitespaces, and with the ES6 syntax by start using the arrow syntax. On the pushPayload: there has reported a bug already for this issue, see #38. Is there more missing? Then we could start by making issues for those.

Sticking on every release is fine with me. See my comment on how to deal with versions in the current readme.

frederikbosch commented 9 years ago

@sebweaver And just added a test for the query bugfix, together with a new release 1.13.1.