search

superfeedr / indexeddb-backbonejs-adapter

An indexedDB adapter for Backbonejs
http://blog.superfeedr.com/indexeddb-backbonejs-adapter/
MIT License
248 stars 61 forks source link

pull request for issue #50 "delete does not handle keyPath" #51

Closed yoann-antoviaque closed 10 years ago

yoann-antoviaque commented 10 years ago

Hi! As you asked here is a pull request fixing the issue I raised.

while writing the test I realized that you used the standard "id" field in your keypath, it was the reason you have not experienced such problems. I changed it to "keyPathId" for the Torrent model, such change raised quite the same error in fetch, therefore I fixed it :)

julien51 commented 10 years ago

Sorry, but it looks like the tests do not pass, so I won't merge :(

yoann-antoviaque commented 10 years ago

which tests do not pass ?

I have a test maked as failed on Chrome ("Use AjaxSync when no database is specified") and a "global failure" in firefox on the unpatched version...

The two tests using keypath on the patched version are ok while failed on the base revision (when keypath is not "id")

I'm willing to help but as tests seem to fail on your own code on an unrelated test case I do not understand... If I missed something or your results are not the same, I would be glad to fix some issue with my modification but cann't see it...

julien51 commented 10 years ago

The travis tests don't pass in here pull request while they do in master.

yoann-antoviaque commented 10 years ago

could you be more specific about which tests do not pass ?

I'm executing tests/tests.html in chrome and firefox and here are the results:

head revision from your repo on chrome: head-chrome on firefox: head-firefox

first revision on my pull request (modified tests with keypath != 'id', untouched backbone-indexeddb.js) on chrome: newtest-chrome on firefox: newtest-firefox

head revision on my pull request on chrome: patched-chrome on firefox: patched-firefox

from my view you have few tests that do not pass depending on the browser (support for the 'addIndividually' property and use ajaxsync when no database is specified on chrome, create duplicate models on firefox) on the head revision, those still fail with my patched version but no other test fails... what am I missing ?

julien51 commented 10 years ago

Yoann, as you see we use Travis on this repo. The master branch passes the test as indicated by screenshot 2014-01-30 21 51 25 Your PR does not build on the other as indicated by screenshot 2014-01-30 21 52 09

I am really sorry I don't have time to dig further at this point. If you make the Travis tests to pass, the, I'll happily trust and merge.