Skip to main content

Bug Tracker

Side navigation

#6768 closed enhancement (fixed)

Opened July 05, 2010 07:35PM UTC

Closed November 02, 2010 03:25AM UTC

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

Attachments (0)
Change History (4)

Changed October 14, 2010 03:16AM UTC by addyosmani comment:1

need: PatchReview
owner: → johncrenshaw
priority: → undecided
status: newpending

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

Changed October 14, 2010 05:11AM UTC by johncrenshaw comment:2

_comment0: 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.1287033205397614
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.

Changed October 15, 2010 09:11PM UTC by snover comment:3

keywords: → needsreview
milestone: 1.4.3
priority: undecidedlow
status: newopen

Changed November 02, 2010 03:25AM UTC by dmethvin comment:4

keywords: needsreview
resolution: → fixed
status: openclosed

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