Make WordPress Core

Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#51519 closed defect (bug) (fixed)

Remove use of jQuery ready()

Reported by: wpnomad's profile wpnomad Owned by: hellofromtonya's profile hellofromTonya
Milestone: 5.9 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch has-unit-tests commit
Focuses: javascript Cc:

Description

ready() has been [deprecated](https://api.jquery.com/ready/), but we still use it in some places:

  • wp-content/themes/twentytwenty/assets/js/customize.js

(and maybe more).

Can this be reviewed? A report to change the same in WooCommerce core is here: https://github.com/woocommerce/woocommerce/issues/27945

Change History (23)

#1 @sabernhardt
4 years ago

  • Focuses javascript added
  • Keywords needs-patch added

Yes, several admin scripts and most bundled themes use .ready() at least once.

Related: #37110 and #50564

#2 @malthert
4 years ago

I reported this originally for WP (& also the WooCommerce issue is from me) - I think in fact we should not only change the .ready() event, but also include info about this change somewhere prominently, so all plugin authors are aware that they should change it too.

The process for this has already been started for all plugins sold on WooCommerce, so this will be finished before WP5.6 is released for those at least.

#3 @SergeyBiryukov
3 years ago

  • Milestone changed from Awaiting Review to 5.8

It looks like most of jQuery(document).ready() instances were removed in [50547] / #51812, but some are still left in these files:

  • js/_enqueues/vendor/mediaelement/wp-playlist.js
  • js/_enqueues/vendor/plupload/handlers.js
  • js/_enqueues/vendor/swfupload/handlers.js
  • js/_enqueues/vendor/thickbox/thickbox.js

These are all either WordPress files (and not external dependencies) or "adopted" libraries that have been patched in the past (in the case of Thickbox), so should be safe to patch.

#4 follow-up: @SergeyBiryukov
3 years ago

There are also a few instances in bundled themes:

  • wp-content/themes/twentyeleven/inc/theme-options.js
  • wp-content/themes/twentyeleven/js/showcase.js
  • wp-content/themes/twentyfifteen/js/functions.js
  • wp-content/themes/twentyfourteen/js/featured-content-admin.js
  • wp-content/themes/twentyseventeen/assets/js/global.js
  • wp-content/themes/twentysixteen/js/functions.js
  • wp-content/themes/twentytwenty/assets/js/customize.js
  • wp-content/themes/twentytwenty/assets/js/customize-preview.js

These may be harder to remove, as the themes must remain compatible with older WP versions they were released in, and possibly one or two versions before that.

#5 in reply to: ↑ 4 @peterwilsoncc
3 years ago

Replying to SergeyBiryukov:

These may be harder to remove, as the themes must remain compatible with older WP versions they were released in, and possibly one or two versions before that.

The jQuery( callback ) technique was added in jQuery 1.0 prior to WordPress's introduction of jQuery 1.1 in [4904] so I think removing the jQuery( document ).ready() statement on the older themes should be fine.

If I am missing something then the older themes may be able to use something along the lines of:

if ( document.readyState !== 'loading' ) {
        readyCallback();
} else {
        window.addEventListener( 'DOMContentLoaded', readyCallback, false );
}

#6 @Clorith
3 years ago

That's correct, it shouldn't be any concern replacing their use in the themes as well without the readyState workaround

This ticket was mentioned in PR #1190 on WordPress/wordpress-develop by pramodjodhani.


3 years ago
#7

  • Keywords has-patch has-unit-tests added; needs-patch removed

Replace jQuery ready() with the alternate syntax $( function() {} ) because ready() has been deprecated.

Trac ticket:https://core.trac.wordpress.org/ticket/51519

Clorith commented on PR #1190:


3 years ago
#8

Thank you for the PR, it does look like there's some accidentally included extra files related to PHP, namely the src/wp-includes/assets/script-loader-packages.php and tests/phpunit/data/link-manager.zip.

Would you be able to refresh your PR to not include these unrelated files?

desrosj commented on PR #1190:


3 years ago
#9

@pramodjodhani are you able to refresh the PR? Looks like there are now merge conflicts.

#10 @desrosj
3 years ago

  • Keywords needs-refresh added

pramodjodhani commented on PR #1190:


3 years ago
#11

Hello @Clorith and @desrosj,

Sorry for the delay. Please can you check again?

#12 @desrosj
3 years ago

@pramodjodhani Thanks for the work! I did push an update to your branch to resolve the conflict with the scripts file.

It looks like there are still a few more instances that need replacing ($(document).ready without spaces):

There's also one inline documentation comment that references this old behavior.

#13 @hellofromTonya
3 years ago

  • Milestone changed from 5.8 to 5.9

Today is 5.8 Beta 1. As there's instances needing replacement, punting to 5.9.

This ticket was mentioned in Slack in #core by chaion07. View the logs.


3 years ago

#15 @chaion07
3 years ago

Thanks @wpnomad for reporting this. We reviewed the ticket during a recent tirage session. We feel that the patch needs to be update to replace the 'ready' in the PHP files. Thanks!

Props to @torounit

This ticket was mentioned in PR #1908 on WordPress/wordpress-develop by costdev.


3 years ago
#16

  • Keywords needs-refresh removed

This PR replaces variations of $(document).ready() and jQuery(document).ready() with the recommended syntax.

Below is a list of files that aren't modified in this PR and the reasoning.

Third-party dependencies:

  • src/js/_enqueues/vendor/mediaelement/mediaelement-and-player.js
  • src/js/_enqueues/vendor/mediaelement/wp-playlist.js
  • src/js/_enqueues/vendor/plupload/handlers.js
  • src/js/_enqueues/vendor/swfupload/handlers.js
  • src/js/_enqueues/vendor/swfupload/handlers.min.js
  • src/js/_enqueues/vendor/thickbox/thickbox.js
  • src/wp-includes/js/jquery/jquery.form.js - Occurs in a comment with a URL that contains the old syntax.

Trac ticket: https://core.trac.wordpress.org/ticket/51519

Presskopp commented on PR #1908:


3 years ago
#17

Do we write jQuery(function($) or jQuery( function( $ )? Both are to find in the commit..

Code is Poetry.

costdev commented on PR #1908:


3 years ago
#18

Do we write jQuery(function($) or jQuery( function( $ )? Both are to find in the commit..

Code is Poetry.

jQuery(function( $ ) - 1 result in 1 file. "It's the taking part that counts."
jQuery(function($) - 6 results in 4 files. "You got a medal! Woo!"
jQuery( function( $ ) - 14 results in 14 files. "A worthy opponent..."
jQuery( function($) - 18 results in 18 files. "Winner!"

This PR now replaces variations of $(document).ready()/jQuery(document).ready() with the appropriate variation for consistency with the majority of the codebase.

{{{js
jQuery( function($) {

Code.

} )

jQuery( function() {

Code.

} )

$( function() {

Code.

} )
}}}

An exception is made where the code is minified. In this case, the appropriate variation is used.

{{{js
jQuery(function($){})
jQuery(function(){})
$(function(){})
}}}

For any wider-reaching work, I suggest that the JS coding standards are evaluated and a proposal is made for a consistent style.

#19 @hellofromTonya
3 years ago

  • Keywords commit added

Marking PR 1908 for commit. It includes additional instances and does not include external vendor packages changes.

#20 @hellofromTonya
3 years ago

  • Owner set to hellofromTonya
  • Status changed from new to reviewing

Self-assigning for commit.

#21 @hellofromTonya
3 years ago

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

In 52285:

External Libraries: Further fix jQuery deprecations in WordPress core.

Follow-up to [50001], [50270], [50367], [50383], [50410], [50420], [50429], [50547].

Props chaion07, Clorith, costdev, desrosj, malthert, peterwilsoncc, presskopp, promz, sabernhardt, SergeyBiryukov, toro_unit, wpnomad.
Fixes #51519.

hellofromtonya commented on PR #1190:


3 years ago
#23

Committed PR #1908 (see changeset https://core.trac.wordpress.org/changeset/52285) instead of this PR. Why? It includes the non-vendor changes + additional fixes for other instances.

Note: See TracTickets for help on using tickets.