Skip to content
Permalink
Browse files

Refactor: Data.prototype.access. Thanks to @RubyLouvre and @gibson042.

…Closes #1167
  • Loading branch information...
rwaldron committed Feb 13, 2013
1 parent b734666 commit 93043d002a5ec8646cbd9d5658434848a1d07a49
Showing with 51 additions and 24 deletions.
  1. +25 −16 src/data.js
  2. +26 −8 test/unit/data.js
@@ -108,24 +108,33 @@ Data.prototype = {
cache : cache[ key ];
},
access: function( owner, key, value ) {
if ( value === undefined && (key && typeof key !== "object") ) {
// Assume this is a request to read the cached data
// In cases where either:
//
// 1. No key was specified
// 2. A string key was specified, but no value provided
//
// Take the "read" path and allow the get method to determine
// which value to return, respectively either:
//
// 1. The entire cache object
// 2. The data stored at the key
//
if ( key === undefined ||
((key && typeof key === "string") && value === undefined) ) {
return this.get( owner, key );
} else {

// If only an owner was specified, return the entire
// cache object.
if ( key === undefined ) {
return this.get( owner );
}

// Allow setting or extending (existing objects) with an
// object of properties, or a key and val
this.set( owner, key, value );
return value !== undefined ? value : key;
}
// Otherwise, this is a read request.
return this.get( owner, key );

// [*]When the key is not a string, or both a key and value
// are specified, set or extend (existing objects) with either:
//
// 1. An object of properties
// 2. A key and value
//
this.set( owner, key, value );

// Since the "set" path can have two possible entry points
// return the expected data based on which path was taken[*]
return value !== undefined ? value : key;

This comment has been minimized.

Copy link
@timmywil

timmywil Feb 13, 2013

Member

Other getters and setters have been switched to be based on arguments.length. Should that be done here?

This comment has been minimized.

Copy link
@rwaldron

rwaldron Feb 13, 2013

Author Member

No. It will break, because the named value parameter will "exist" and actually be undefined because the bindings were created in the exposed jQuery.data or jQuery.fn.data call:

function access( a, b, c ) { 
  return arguments.length; 
}
function data( a, b, c ) { 
  // a, b, c are now bound identifiers
  return access(a, b, c); 
}

data("a");
// 3
data("a", "b");
// 3
data("a", "b", "c");
// 3

Besides, we should change all code away from any arguments object dependency, wherever possible.

This comment has been minimized.

Copy link
@dmethvin

dmethvin Feb 13, 2013

Member

We've been headed in the opposite direction for a few versions now, it's sometimes useful to know whether undefined was explicitly passed especially when it makes the difference between a getter and a chain. But I see the issue here.

This comment has been minimized.

Copy link
@rwaldron

rwaldron Feb 13, 2013

Author Member

It's not an issue. The data API supports exactly what it always supported and has tests to back it up...

// snip
jQuery.data(elem, "foo", undefined);
equal( jQuery.data(elem, "foo"), "baz", "Data is not unset by passing undefined to jQuery.data" )

// snip
strictEqual( div.data("test", undefined).data("test"), "overwritten", ".data(key,undefined) does nothing but is chainable (#5571)");
},
remove: function( owner, key ) {
var i, l, name,
@@ -7,29 +7,47 @@ test("expando", function(){
});

test( "jQuery.data & removeData, expected returns", function() {
expect(2);
expect(4);
var elem = document.body;

equal(
jQuery.data( document.body, "hello", "world" ), "world",
jQuery.data( elem, "hello", "world" ), "world",
"jQuery.data( elem, key, value ) returns value"
);
equal(
jQuery.removeData( document.body, "hello" ), undefined,
jQuery.data( elem, "hello" ), "world",
"jQuery.data( elem, key ) returns value"
);
deepEqual(
jQuery.data( elem, { goodnight: "moon" }), { goodnight: "moon" },
"jQuery.data( elem, key, obj ) returns obj"
);
equal(
jQuery.removeData( elem, "hello" ), undefined,
"jQuery.removeData( elem, key, value ) returns undefined"
);

});

test( "jQuery._data & _removeData, expected returns", function() {
expect(2);
expect(4);
var elem = document.body;

equal(
jQuery._data( document.body, "hello", "world" ), "world",
"jQuery.data( elem, key, value ) returns value"
jQuery._data( elem, "hello", "world" ), "world",
"jQuery._data( elem, key, value ) returns value"
);
equal(
jQuery._removeData( document.body, "hello" ), undefined,
"jQuery.removeData( elem, key, value ) returns undefined"
jQuery._data( elem, "hello" ), "world",
"jQuery._data( elem, key ) returns value"
);
deepEqual(
jQuery._data( elem, { goodnight: "moon" }), { goodnight: "moon" },
"jQuery._data( elem, obj ) returns obj"
);
equal(
jQuery._removeData( elem, "hello" ), undefined,
"jQuery._removeData( elem, key, value ) returns undefined"
);
});

0 comments on commit 93043d0

Please sign in to comment.
You can’t perform that action at this time.