axemclion / jquery-indexeddb

An IndexedDB Plugin for Jquery.
Other
195 stars 71 forks source link

putting objects should be consistent across IndexedDB implementations #11

Closed pixelcort closed 11 years ago

pixelcort commented 11 years ago

I have an object store created using the following setup code:

$.indexedDB('helloworld', {
  version: 1,
  upgrade: function(transaction) {
    var items = transaction.createObjectStore('items', {
      autoIncrement: false,
      keypath: 'id',
      keyPath: 'id'
    });
  }
});

Then, when I go to put items, I have to change how I call put based on the underlying IndexedDB implementation.

For Firefox, I have to explicitly specify the key as the second argument, or the done callback won't get called:

$.indexedDB('helloworld').objectStore('items').put({id:'theKey',from:'firefox'}, 'theKey').done(function(){console.log('d');});

However, when using IndexedDB Shim in Safari, I have to not provide the second argument to avoid a thrown error within the shim:

$.indexedDB('helloworld').objectStore('items').put({id:'theKey',from:'safari'}).done(function(){console.log('d');});

Instead, the jQuery plugin for IndexedDB should be aware of the differences in IndexedDB implementations, and automatically change how it's calling the underlying API so code calling the jQuery plugin can be consistent.

Workaround is to not specify a keyPath and just always provide it separately.

pixelcort commented 11 years ago

On the shim side, it looks like there is also a related issue https://github.com/axemclion/IndexedDBShim/issues/7

axemclion commented 11 years ago

Seems to work for me in safari - I tried

$.indexedDB('helloworld', {
  version: 1,
  upgrade: function(transaction) {
    var items = transaction.createObjectStore('items', {
      autoIncrement: false,
      keypath: 'id',
      keyPath: 'id'
    });
  }
});

$.indexedDB('helloworld').objectStore('items').put({id:'theKey',from:'safari'}).done(function(){console.log('d');});

Are you still seeing this issue ?

axemclion commented 11 years ago

Also, you should not have to use keyPath and keypath - that has been normalized with the change in the Shim, has it not ? IMHO, the jquery plugin should do only what the standards does - i.e. the Firefox like behavior. Not sure if it is a good idea for this jquery library to know a difference due to the polyfill.

pixelcort commented 11 years ago

Whoops, ignore the keyPath vs keypath thing; that's a typo on my part.

I think you interpreted the issue backwards. Safari works fine when the second argument is not specified. But when I run the following in Safari:

$.indexedDB('helloworld').objectStore('items').put({id:'theKey',from:'safariWithSeparateKeyArgumentWillFail'}, 'theKey').done(function(){console.log('d');});

I get an error from within the shim and the done callback never gets called.

In Firefox it only works with the second argument being passed to put, which actually seems wrong to me.

What really confuses me is why Firefox would require the key to be provided as a separate argument, if the keyPath was specified. From my limited reading of the spec, it would appear the whole point of keyPath was so that this wouldn't be needed.

Perhaps the bug then is in the implementation of IndexedDB in Firefox requiring the key separately even if keyPath is specified?

axemclion commented 11 years ago

According to the specification, the second argument to the put is optional, and the two error conditions are

In our case above, there is an explicit keyPath and hence, Firefox should throw an error when the second parameter is specified. Looks like this is a bug in the jquery implementation that I am investigating.

Will keep you updated.

axemclion commented 11 years ago

Here are the test cases for IndexedDB put - https://gist.github.com/3735483

I think the way jquery-indexeddb works if perfectly fine, as expected in the specification. Firefox and Chrome have the right implementation.

I think the issue is with the IndexedDBSHim only. We should close this issue here and track this issue with the IndexedDBShim project only

pixelcort commented 11 years ago

Okay, closing in agreement that any change would go in the shim instead.