Bug Tracker

Opened 9 years ago

Closed 9 years ago

#6768 closed enhancement (fixed)

Patch for ready() (Corrected submission)

Reported by: johncrenshaw Owned by: johncrenshaw
Priority: low Milestone:
Component: core Version: 1.4.2
Keywords: Cc:
Blocked by: Blocking:

Description

Previously submitted a couple of small fixes for ready() in the jquery-ui repository (rather than jquery). Resubmittin in the right place:

http://github.com/johncrenshaw/jquery/commit/401fc6abd20f5e9f1a9534d10defc8af8735a138

Change History (4)

comment:1 Changed 9 years ago by addyosmani

need: PatchReview
Owner: set to johncrenshaw
Priority: undecided
Status: newpending

Could you please go into deeper detail on how these changes may improve the performance of ready()?

comment:2 Changed 9 years ago by johncrenshaw

Status: pendingnew

As previously written, ready() cannot be extended very well. My changes removed some arbitrary limitations to make this more realistic.

My specific use case was an implementation of $.include() that could be used in conjunction with AJAX. $.include() would include the files and wait for them to load. Meanwhile, $.ready(function(){...}) could be used as normal, but would defer until the relevant includes were fully loaded. The basic idea was to make everything work consistently (especially ready()) even when content was loaded via AJAX, but without requiring all script files to be loaded at page load time. But I digress.

In doing the above, it became necessary to extend ready(). I know I'm not the first person to do this, (I believe previous versions of the live query plugin did this?) and I'm sure I won't be the last.

In version 1.4.2 ready() changed in ways that made it difficult/impossible/unsafe to reliably extend as needed. My patch includes 4 small tweaks that fix this. The code style is improved, and it becomes trivial to safely (and reliably) extend ready().

The changes and reasons are explained in comments in the code, but I'll cover them here too:

1) I added an _onReady function as a stub for ready(). This way addEventListener can bind to _onReady, and anyone overriding ready() doesn't also need to repair the listener binding. (At best, repairing the binding requires a complex and nasty kludge.)

2) Previously, the code used readyList = null to reset the list of ready functions. This is technically an error, and prevents the list from being reused if needed (such as when writing a robust include handler.) I changed this to reset using readyList = [] instead, which keeps the variable contents consistent, and opens doors to future possibilities.

3) and 4) I changed the addEventListener events to bind to _onReady (instead of binding directly to ready, which is what they did before.) See 1 for an explanation of this.

These changes don't improve performance, they correct seemingly minor style issues that became a huge limitation later when writing a plugin. The edits are trivial and don't affect the normal operation of jQuery.

Last edited 9 years ago by johncrenshaw (previous) (diff)

comment:3 Changed 9 years ago by snover

Keywords: needsreview added
Milestone: 1.4.3
Priority: undecidedlow
Status: newopen

comment:4 Changed 9 years ago by dmethvin

Keywords: needsreview removed
Resolution: fixed
Status: openclosed

This was implemented in 1.4.3, not sure if it was based on this code or not.

Note: See TracTickets for help on using tickets.