Make WordPress Core

Opened 9 years ago

Closed 7 years ago

#32358 closed feature request (wontfix)

Add unminified jQuery to core for better debugging with SCRIPT_DEBUG enabled

Reported by: crazyjaco's profile CrazyJaco Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Script Loader Keywords: has-patch ongoing needs-unit-tests
Focuses: Cc:

Description

I looked through old trac tickets to see if anyone proposed this in the past, but didn't find anything.

Most of the scripts included in core have both minified and unminified versions. By default the minified scripts are used. When SCRIPT_DEBUG is set to true, the unminified scripts will be used.

There is no unminified version of jQuery-core in WordPress core. That should be added.

I'd like to submit a patch for this myself

Attachments (4)

32358.patchโ€‹ (476.3 KB) - added by CrazyJaco 9 years ago.
First pass. Upgraded jQuery from 1.11.1 to 1.11.3. Added unminified file and changed script-loader entry.
32358.2.patchโ€‹ (474.3 KB) - added by CrazyJaco 9 years ago.
Realized it was probably in poor taste to upgrade jQuery at the same as implementing this. Changed it back to jQuery 1.11.1
32358.3.patchโ€‹ (476.4 KB) - added by CrazyJaco 9 years ago.
Add unminified and minified versions of jquery and adjusted script-loader.php appropriately. No conflict mode added to jquery files.
32358.diffโ€‹ (490.0 KB) - added by swissspidy 8 years ago.

Download all attachments as: .zip

Change History (32)

@CrazyJaco
9 years ago

First pass. Upgraded jQuery from 1.11.1 to 1.11.3. Added unminified file and changed script-loader entry.

@CrazyJaco
9 years ago

Realized it was probably in poor taste to upgrade jQuery at the same as implementing this. Changed it back to jQuery 1.11.1

#1 @CrazyJaco
9 years ago

  • Keywords has-patch added

#2 @johnbillion
9 years ago

  • Keywords dev-feedback added
  • Milestone changed from Awaiting Review to Future Release

+1.

Any objections? There may well be a reason this hasn't been done before.

#3 @netweb
9 years ago

Related: #32793

#4 @peterwilsoncc
9 years ago

  • Keywords needs-refresh added

+1

jQuery was upgraded to 1.11.2 in #31423 [31567] and #32794 exists for upgrading to 1.11.3.

#5 follow-up: @CrazyJaco
9 years ago

Just saw this update. Would the first patch suffice since that was originally upgrading to 1.11.3 as well?

#6 in reply to: โ†‘ย 5 @peterwilsoncc
9 years ago

Replying to CrazyJaco:

Would the first patch suffice since that was originally upgrading to 1.11.3 as well?

I'm afraid it will need a new patch, the original file refers to lines that no longer exist so will not apply correctly.

Additionally, WordPress runs jQuery in noConflict mode which has been removed in your patch. Following jQuery in the new file, you will need to add the line jQuery.noConflict();.

@CrazyJaco
9 years ago

Add unminified and minified versions of jquery and adjusted script-loader.php appropriately. No conflict mode added to jquery files.

#7 @CrazyJaco
9 years ago

  • Keywords needs-refresh removed

Thank you for pointing out that noConflict() had been added to the jquery file. That was not something I had ever noticed. I knew it was in effect, but not where it was declared. I added it to both files in the patch.

This ticket was mentioned in โ€‹Slack in #core by crazyjaco. โ€‹View the logs.


9 years ago

#9 follow-up: @chriscct7
9 years ago

  • Keywords dev-feedback removed
  • Milestone changed from Future Release to 4.4
  • Owner set to chriscct7
  • Status changed from new to reviewing
  • Type changed from enhancement to feature request
  • Version 4.3 deleted

Starting a review of this. Will post my findings tomorrow.

Versions shouldn't be defined on enhancements since a version defines when an "issue" starts.

#10 in reply to: โ†‘ย 9 @SergeyBiryukov
9 years ago

Replying to chriscct7:

Versions shouldn't be defined on enhancements since a version defines when an "issue" starts.

It's fine, version can also indicate when the enhancement was initially suggested.

This ticket was mentioned in โ€‹Slack in #core by crazyjaco. โ€‹View the logs.


9 years ago

#12 @chriscct7
9 years ago

  • Status changed from reviewing to accepted

It looks good, if there's interest in putting it in. Not too sure on the benefit it provides

#13 @CrazyJaco
9 years ago

Nearly every other javascript file in the script-loader accounts for an unminified version for debugging. Why would jQuery not qualify for same?

I don't understand why jQuery was left out, but I do think this adds significant value.

#14 follow-up: @azaozz
9 years ago

Not sure if this will be particularly helpful. We definitely don't want the non-minified file in /build. It will increase the download size needlessly.

It can be added to /src and not copied when building (similarly to tinymce.js), but the benefits are somewhat unclear, unless we want to debug jQuery itself :)

There are so many places with jQuery help, tips and troubleshooting that searching for the exact error message is more beneficial than knowing the line where it breaks.

#15 @wonderboymusic
8 years ago

  • Keywords ongoing added
  • Milestone changed from 4.4 to Future Release

I'm gonna kick the can down the road here - I don't see us bundling the un-min'd file, but I don't want to close down discussion either

#16 @chriscct7
8 years ago

  • Owner chriscct7 deleted
  • Status changed from accepted to assigned

#17 @swissspidy
8 years ago

#25244 was marked as a duplicate.

#18 @ericlewis
8 years ago

It can be added to /src and not copied when building (similarly to tinymce.js), but the benefits are somewhat unclear, unless we want to debug jQuery itself :)

When debugging JS, I often to step through bundled library code to watch the control flow and inspect state.

#19 @peterwilsoncc
8 years ago

  • Keywords needs-refresh added

Needs refresh following [36285] / #35380

#20 in reply to: โ†‘ย 14 ; follow-up: @iandunn
8 years ago

Replying to azaozz:

Not sure if this will be particularly helpful. [...] searching for the exact error message is more beneficial than knowing the line where it breaks.

I don't think that's always true; sometimes I run into situations during plugin development where I get errors like TypeError: n is undefined. Knowing the real variable name and line number would give some insight into the underlying problem.

#21 in reply to: โ†‘ย 20 ; follow-up: @dotancohen
8 years ago

Replying to iandunn:

I don't think that's always true; sometimes I run into situations during plugin development where I get errors like TypeError: n is undefined. Knowing the real variable name and line number would give some insight into the underlying problem.

This is what brought me here. One cannot debug without knowing exactly where the error was thrown. The minified versions obscure that information, specifically the line number and the variable names.

In regards to increasing download size, the uncompressed jQuery 2.2.1 file is under 300 Kib:
โ€‹http://code.jquery.com/jquery-2.2.1.js

That will compress to well below 100 KiB. Adding 100 KiB to the 7.4 MiB (zipped) or 6.8 MiB (tarball) Wordpress 4.4.2 is beyond negligible. If size really were a concern, the rest of Wordpress (PHP) could be minified with an order of magnitude better result.

@swissspidy
8 years ago

#22 @swissspidy
8 years ago

  • Keywords needs-unit-tests added; needs-refresh removed

Now that wp_add_inline_script() is available (see #14853), the jQuery.noConflict(); part is easy to solve.

I think we can easily add the non-minified file in /src and only ship the minified file in /build. That way file size is not a big deal. Sounds like a good compromise?

32358.diffโ€‹ is a proof-of-concept patch (using jQuery 1.12.1). Of course some tests would need to be added and some altered.

#23 in reply to: โ†‘ย 21 ; follow-up: @azaozz
8 years ago

Replying to dotancohen:

Replying to iandunn:

I don't think that's always true; sometimes I run into situations during plugin development where I get errors like TypeError: n is undefined. Knowing the real variable name and line number would give some insight into the underlying problem.

This is what brought me here. One cannot debug without knowing exactly where the error was thrown. The minified versions obscure that information, specifically the line number and the variable names.

And what would you do next after knowing the line number and eventually the var name? Look at the trace to see which of your functions called jQuery of course. I bet the error will always be in your function not in the library.

And what is stopping you do the same when jQuery is minified? Well, nothing really. The trace is the part you really want/need to debug it :)

That will compress to well below 100 KiB. Adding 100 KiB to the 7.4 MiB (zipped) or 6.8 MiB (tarball) Wordpress 4.4.2 is beyond negligible.

Not true in principle. Why would you want to force 20,000,000 - 30,000,000 people to download these ~100k every time WordPress is updated? (Leaving the "terabytes of wasted bandwidth" calculation out.)

I don't mind adding the non-minified jQuery to /src. Still think it is mostly pointless. The only user case is when somebody hits a bug in it at which point they can probably download the non-minified version and replace the file in their test install. We can save them the 2-3 min that would usually take.

Last edited 8 years ago by azaozz (previous) (diff)

#24 @futtta
8 years ago

This ticket is also important for JS minification plugins; currently by default they will try to re-minify jquery.js (which is relatively "costly", CPU-wise), as they only skip minification if the ".min" suffix (or "-min") is in the filename.

So ideally we indeed have jquery.js (new, unminified) and jquery.min.js (minified, currently named jquery.js), with that last one being loaded by default.

#25 in reply to: โ†‘ย 23 @iandunn
8 years ago

Replying to azaozz:

And what would you do next after knowing the line number and eventually the var name? Look at the trace to see which of your functions called jQuery of course. I bet the error will always be in your function not in the library.

And what is stopping you do the same when jQuery is minified? Well, nothing really. The trace is the part you really want/need to debug it :)

I think that's true in most cases, but maybe not all. IIRC, the trace wasn't enough for the situation that prompted me to write that comment, but it's been too long to remember the details, and offhand, I can't think of another situation that would fit.

It's definitely possible that I just had a brain fart that day and didn't think to look at the trace, but I'll keep an eye out for similar situations in the future, and leave a comment here with details if I find one.

#26 follow-up: @superpoincare
8 years ago

There are plugins which move jquery to the footer or minify them with other files or both.

Won't adding jQuery.noConflict() to wp_add_inline_script() complicate things?

Last edited 8 years ago by superpoincare (previous) (diff)

#27 in reply to: โ†‘ย 26 @swissspidy
8 years ago

Replying to superpoincare:

There are plugins which move jquery to the footer or minify them with other files or both.

Won't adding jQuery.noConflict() to wp_add_inline_script() complicate things?

Moving to the footer is not a problem. The jQuery name obviously needs to be kept after minification, otherwise everything would break. So that's not an issue either. The real question is: Do these plugins properly support wp_add_inline_script()? For example, WordPress combines scripts and styles in the admin, and wp_add_inline_script() works fine there. Plugins should really support that too and I think they do. Otherwise we would have heard by now.

Feel free to test the patch with your favourite caching/minification plugin :)

#28 @peterwilsoncc
7 years ago

  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from assigned to closed

Closing this off as wontfix due to a lack of enthusiasm for bundling uncompressed libraries.

Note: See TracTickets for help on using tickets.