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

[1.9 vote] jQuery.fn.data( callback( index, data ) ) - Fixes #12397 #910

Closed
wants to merge 2 commits into from

Conversation

gnarf
Copy link
Member

@gnarf gnarf commented Aug 25, 2012

Sizes - compared to master
    258888      (+167)  dist/jquery.js                
     92841       (+60)  dist/jquery.min.js            
     33218       (+11)  dist/jquery.min.js.gz  

Proposal for a new signature to jQuery.fn.data()

Trac Ticket: http://jqbug.com/12397

@rwaldron
Copy link
Member

+1, Nice consistency with the rest of proto method APIs

What about something like...

$( selector ).data(function( k , data ) {
  return {
    a: data.a || "alpha",
    b: "beta"
  };
});

@gnarf
Copy link
Member Author

gnarf commented Aug 25, 2012

The return isn't happy, for consistency, the data object itself must be modified instead of assigning a new object. You could just extend tho...

$( selector ).data(function( k , data ) {
  jQuery.extend( data, {
    a: data.a || "alpha",
    b: "beta"
  });
});

I think making the backend of the API do this would just pollute it, would people would also expect missing keys to be deleted?

@gnarf
Copy link
Member Author

gnarf commented Aug 25, 2012

Dealing with two conflicting patterns here, the fn.text( function( index, oldText ) { return newText; }) and the fact that fn.data() should NEVER change in the lifetime of that object, only its properties.

It might make sense to pass the return value into elem.data( obj ); ? I think its easier to have it be like each, and just ignore the return value, perhaps have a return false exit?

@gnarf
Copy link
Member Author

gnarf commented Aug 26, 2012

@rwldrn - If we wanted to try to be consistent with the other collection setters it might look something like this:

if ( jQuery.isFunction( key ) ) {
    return this.each( function( index ) {
        var result = key( index, jQuery( this ).data() );
        if ( typeof result === "object" ) {
            jQuery( this ).data( result );
        }
    });
}

10 bytes heavier on the gzip size - perhaps useful?

@rwaldron
Copy link
Member

Sorry, I was both +1'ing the addition for being consistent and separately asking about API possibilities. I was and still am mobile, so I hadn't actually tested that, just tapped it out ;)

@rkatic
Copy link
Contributor

rkatic commented Aug 26, 2012

@gnarf37 Using the returned value to update data seems consistent with other APIs, but the typeof result === "object" check seems wrong. Why not result !== undefined?

// Calls callback with index, data
if ( jQuery.isFunction( key ) ) {
return this.each( function( index ) {
key.call( this, index, jQuery( this ).data() );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better use $.data() instead, for performance reasons...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency we can't. It must pass through the chunk of code that can read html attrs.

@gnarf
Copy link
Member Author

gnarf commented Aug 26, 2012

@rkatic If a string, boolean, etc is returned, it can't be set to "data" -- only objects can be extended onto the data object.

@rkatic
Copy link
Contributor

rkatic commented Aug 26, 2012

@gnarf37 Right... for a moment I supposed it was about jQuery.fn.data( name, callback( index, value ) ). My bad.

Btw. also null can not be extended.

@dmethvin
Copy link
Member

Not voted in, see ticket.

@dmethvin dmethvin closed this Oct 30, 2012
@lock lock bot locked as resolved and limited conversation to collaborators Jan 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants