Bug Tracker

Ticket #14659 (closed feature: fixed)

Opened 11 months ago

Last modified 9 months ago

Avoid use of Object.keys() in cleanData()

Reported by: hiroshi Owned by: m_gol
Priority: low Milestone: 2.1.1
Component: manipulation Version: 2.1.0-beta2
Keywords: Cc:
Blocking: Blocked by:

Description

Object.keys() is still not supported by some old browsers.  http://kangax.github.io/es5-compat-table/

Also for in is used to iterate event types in other places (i.e. jQuery.event.remove).

for ( type in events ) {
	jQuery.event.remove( elem, type + types[ t ], handler, selector, true );
}

I think we should avoid use of Object.keys() in terms of followings.

  • Browser compatibility
  • Code consistency

Change History

comment:1 Changed 11 months ago by rwaldron

Object.keys() is still not supported by some old browsers.

This is not a relevant "issue" for jQuery 2.x, which only supports browsers that _will_ have Object.keys

comment:2 Changed 11 months ago by dmethvin

I think the consistency issue is a better argument, since we don't use Object.keys anywhere else. However, the good thing about it would be that potentially we could survive augmentation of Object.prototype if we used this everywhere, since it only enumerates own properties? The for/in makes the code a bit shorter and simpler, I haven't looked to see the impact on gzip though.

Last edited 11 months ago by dmethvin (previous) (diff)

comment:3 Changed 11 months ago by timmywil

  • Priority changed from undecided to low
  • Status changed from new to open
  • Component changed from unfiled to event
  • Type changed from bug to feature

While this is not high priority, I think I would prefer to use the for-in loop to save a few bytes. That is, if the argument is for consistency, the bytes would add up if we used Object.keys ubiquitously. Supporting Object.prototype augmentation is not something we've pursued historically and I don't think we would want to make the necessary changes to the 1.x branch to keep it consistent with 2.x if we wanted to start.

Last edited 11 months ago by timmywil (previous) (diff)

comment:4 Changed 11 months ago by hiroshi

I added some analysis. Performance and file size seem better.  https://github.com/jquery/jquery/pull/1472

comment:5 Changed 9 months ago by dmethvin

  • Milestone changed from None to 1.11.1/2.1.1

comment:6 Changed 9 months ago by dmethvin

  • Milestone changed from 1.11.1/2.1.1 to 2.1.1

comment:7 Changed 9 months ago by m_gol

  • Owner set to m_gol
  • Status changed from open to assigned

comment:8 Changed 9 months ago by m_gol

  • Component changed from event to manipulation

comment:9 Changed 9 months ago by Michał Gołębiowski

  • Status changed from assigned to closed
  • Resolution set to fixed

Manipulation: don't use Object.keys for consistency

Fixes #14659

Changeset: a96d5bed58f2b30f97d6dd2f5691cd890f62b75f

Note: See TracTickets for help on using tickets.