Skip to content

Event: Warn about late use of $(window).on("load"...) #217

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Event: Warn about late use of $(window).on("load"...) #217

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1776e3d
Event: Warn about late use of $(window).on("load"...)
dmethvin Aug 5, 2016
7f1ff0e
squash! Use jQuery instead of $ in messages
dmethvin Sep 26, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/event.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
var oldLoad = jQuery.fn.load,
oldEventAdd = jQuery.event.add,
originalFix = jQuery.event.fix;

jQuery.event.props = [];
Expand Down Expand Up @@ -35,6 +36,15 @@ jQuery.event.fix = function( originalEvent ) {
return fixHook && fixHook.filter ? fixHook.filter( event, originalEvent ) : event;
};

jQuery.event.add = function( elem, types ) {

// This misses the multiple-types case but that seems awfully rare
if ( elem === window && types === "load" && document.readyState === "complete" ) {
migrateWarn( "jQuery(window).on('load'...) called after load event occurred" );
}
return oldEventAdd.apply( this, arguments );
};

jQuery.each( [ "load", "unload", "error" ], function( _, name ) {

jQuery.fn[ name ] = function() {
Expand Down
37 changes: 37 additions & 0 deletions test/event-lateload.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8">
<title>jQuery Migrate late load event binding test</title>

<!-- Load a jQuery and jquery-migrate plugin file based on URL -->
<script src="testinit.js"></script>
<script>
TestManager.loadProject( "jquery", "git" );
</script>
<script src="iframeTest.js"></script>
<script>
jQuery.noConflict();
TestManager.loadProject( "jquery-migrate", "dev", true );
</script>
<script>
var loaded = jQuery.Deferred();

// No warning here, document isn't yet loaded
jQuery( window ).on( "load", function() {
loaded.resolve();
});

jQuery.when( jQuery.ready, loaded ).then( function() {

// This .on() call should warn
jQuery( window ).on( "load", jQuery.noop );

startIframeTest();
} );
</script>
</head>
<body>
<p>jQuery Migrate late load event binding test</p>
</body>
</html>
9 changes: 9 additions & 0 deletions test/event.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,3 +145,12 @@ TestManager.runIframeTest( "jQuery.event.fixHooks", "event-fixHooks.html",
assert.equal( jQuery.migrateWarnings.length, 2, "warnings: " +
JSON.stringify( jQuery.migrateWarnings ) );
} );

TestManager.runIframeTest( "Load within a ready handler", "event-lateload.html",
function( assert, jQuery, window, document, log ) {
assert.expect( 2 );

assert.equal( jQuery.migrateWarnings.length, 1, "warnings: " +
JSON.stringify( jQuery.migrateWarnings ) );
assert.ok( /load/.test( jQuery.migrateWarnings[ 0 ] ), "message ok" );
} );
6 changes: 6 additions & 0 deletions warnings.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,3 +193,9 @@ See jQuery-ui [commit](https://github.com/jquery/jquery-ui/commit/c0093b599fcd58
**Cause:** The code on the page has used the `jQuery.event.props` or `jQuery.event.fixHooks` data structures. These were used in previous versions to affect the properties that are copied from the native event to the jQuery event each time an event is delivered, but they had the potential to create performance issues. Versions of jQuery Mobile before 1.5 make use of this API and require jQuery Migrate to run properly.

**Solution:** The most popular use of these data structures are to add properties for touch or pointer events, and those properties are now supported by default with a newer high-performance approach in jQuery 3.0 that only retrieves the properties on first access. If you are using jQuery Mobile, check the [jquerymobile.com](https://jquerymobile.com) site for updates. If you are using plugins such as [pointerTouch](https://github.com/timmywil/jquery.event.pointertouch) or [touchHooks](https://github.com/aarongloege/jquery.touchHooks), simply remove them as they are no longer needed.

### JQMIGRATE: jQuery(window).on('load'...) called after load event occurred

**Cause:** The calling code has attempted to attach a `load` event to `window` after the page has already loaded. That means the handler will never run and so is probably not what the caller intended. This can occur when the event attachment is made too late, for example, in a jQuery ready handler. It can also occur when a file is loaded dynamically with jQuery after the page has loaded, for example using the `$.getScript()` method.

**Solution:** If a function `fn` does not actually depend on all page assets being fully loaded, switch to a ready handler `$( fn )` which runs earlier and will aways run `fn` even if the script that contains the code loads long after the page has fully loaded. If `fn` actually does depend on the script being fully loaded, check `document.readyState`. If the value is `"complete"` run the function immediately, otherwise use `$(window).on( "load", fn )`.