Bug Tracker

Ticket #7654 (closed bug: wontfix)

Opened 4 years ago

Last modified 4 years ago

cleaning warnings in jQuery

Reported by: arimbourg@… Owned by:
Priority: undecided Milestone: 1.6
Component: unfiled Version: 1.4.4
Keywords: Cc:
Blocking: Blocked by:

Description

I've cleaned the jQuery core code to avoid the warnings that appears in firefox error console (like "function X don't always return a value" or other "undefined properties").

You can access these copies here :  http://hq.ariworld.eu/javascript/jquery-1.4.4-clean.js (and  http://hq.ariworld.eu/javascript/jquery-1.4.2-clean.js )

There is one warning remaining : "Unexpected token in attribute selector: < ! >." from line 4139 (4159 in my cleaned copy)

There may be some more warnings but I noticed only those appearing in my scripts.

I hope next versions could be fully clean...

Change History

comment:1 Changed 4 years ago by snover

  • Status changed from new to closed
  • Resolution set to wontfix

Thanks for the report, but this is not a jQuery bug. Warnings are NOT DANGEROUS and should be ignored. If you can’t deal with ignoring them yourself, please change your error console’s settings so that it only shows errors. We won’t bloat the library to remove superfluous CSS warnings.

comment:2 Changed 4 years ago by arimbourg@…

I know I can disable warnings and that they are not dangerous... but I always prefer writing scripts that don't produce any warnings rather than disabling these warnings. It's always better. Now if you don't want these corrections, do as you want... It's not difficult to avoid them... it only take some time to clean your file for each new release...

comment:3 Changed 4 years ago by rwaldron

I'm just curious... what are we expected to do with those links above? Have you ever looked at the jQuery source code on github.com?

 https://github.com/jquery/jquery

A cursory review of your code reveals nothing to me about what you've cleaned... and I did a control+f for "clean" and found no comments left by you.

comment:4 Changed 4 years ago by ajpiano

 http://paste.pocoo.org/compare/297742/297741/

his change makes two minor tweaks in property detection for jquery on window, and adds "return null" 40 times.

jQuery passes JSLint, it does not need to pass the... "Don't have any Firebug Warnings" test, especially if it just means adding return null to functions that don't need to return anything.

comment:5 Changed 4 years ago by arimbourg@…

To ajpiano

I cleaned jQuery to avoid the strict warnings appearing in firefox error console (not firebug). I added "return null" (or sometimes just "null" to existing "return") to avoid "function don't always return a value" warnings. The functions where I added that returned a value yet in a condition or loop. So don't say that I added "return null to function that don't need ...". A "return;" or a "return null;" or a function without "return;" returns the same value (or that could be "undefined"). I used "null" because I saw that you commonly use it in conditions. It don't change anything to the behavior of jQuery... but it's cleaner (... good practice). I also visited JSLint. The validator is stricter than me and display more warnings than me. (I didn't find the "Don't have any Firebug Warnings" option).

To rwaldron,

I just offered to check them and maybe to include these 'cleanings' for the next versions. If you want, I can check the src files form github and clean them and add comments too...

If you don't want my help, I'll just continue cleaning my copies each time a new version comes...

comment:6 Changed 4 years ago by rwaldron

The changes you've made are across both jQuery and Sizzle. Also, have you run these against the jQuery test suite?

comment:7 Changed 4 years ago by ajpiano

The "Don't have any Firebug Warnings" test was a made up, figurative test that describes the arbitrary goal of having jQuery not produce any Warnings from the Firefox console. It is not a real option for JSLint, which is the tool that jQuery uses for checking code for integrity and style.

This is not a matter of "not wanting your help," which, if offerred for a different issue, we would gladly accept. It is simply that the "reward" of jQuery not producing warnings in the Firefox console is not worth the bulk of adding these changes to jQuery and Sizzle, which may behave unpredictably as they have not been run against the tests.

Thank you for your time and interest in helping the jQuery project!

Last edited 4 years ago by ajpiano (previous) (diff)

comment:8 Changed 4 years ago by arimbourg@…

To rwaldron

I didn't use the jQuery test suite (I didn't even know there's one). Are you talking about " http://jsfiddle.net/" ? I only used my test suite... a remake of window.alert function to test and check results... Just tell me where to find that tool and I'll try it.

To ajpiano

I would be happy to help for the more serious issues. Yesterday, I read some of them. What's the procedure?

Now, I've added comments to the modifications that I made in jQuery (  http://hq.ariworld.eu/javascript/jquery-1.4.4-clean+comments.js ). Reading these comments, you'll understand that it couldn't be any bad issue. Give me the test suite to be sure... or don't if a clean code is really not interesting...

comment:9 Changed 4 years ago by rwaldron

The jQuery test suite is a collection of 3688 unit tests that jQuery must pass in the browsers I listed above before it can be deemed release worthy.

The tests exist to aid in avoiding major unexpected regressions in the jQuery source (unless the regression is somehow intended ) from one release to another. Imagine, i make one small change - like... returning null when nothing was previously returned. No big deal right? Wrong. Javascript functions ALWAYS return something - if it is no specified then the default behaviour is to return undefined. So what happens when some other piece of internal jQuery code is testing a condition in which it expects undefined to the result of its evaluation... but now you're returning null. This is what happens:

16 Failed tests spanning across the data and css modules. Only 2782 tests were allowed to run because the entire suite dies when it gets to the custom JSON-P tests.

So I removed that test block, which is only 2 tests and those count against the total. Now you're failing 18.

This allowed the rest of the suit to run and finish with 68 failures, plus the removed two for a total of 70 failures across the data, css, ajax, effects and dimensions modules.

This test was run in both Chromium 9.0.595.0 and Firefox 4b7 - in both browsers I ran the unmodified test suite to ensure 100% passing. Tomorrow I will run it against the older versions of IE.

Version 0, edited 4 years ago by rwaldron (next)

comment:10 Changed 4 years ago by anonymous

You are right... I wrote that before...

(function () { }) () === undefined

true

(function () { }) () === null

false

(function () { return; }) () === undefined

true

(function () { return; }) () === null

false

(function () { return null; }) () === null

false

(function () { return null; }) () === null

true

(function () { return; }) () === undefined

true

(function () { return; }) () === null

false

(function () { return undefined; }) () === null

false

(function () { return null; }) () === null

true

ok... So let's return undefined...

comment:11 Changed 4 years ago by arimbourg@…

No... it's not enough... I test now qunit...

comment:12 Changed 4 years ago by Bill

I think the jquery developers need to go through and clean this up. Return null or undefined, whatever it takes in each case to get it right. Seems like it's nothing more than lazy/messy coding, and I don't see any technical reason for not fixing this. Having unnecessary warnings is just bad practice, and not acceptable for many professional users.

Perhaps jquery should make it a standard to address things like this before releasing, just like testing.

Don't take me wrong - I love the product and appreciate all the great work the team has done. But please try to understand the perspective of developers who use this in their professional work and thus can't make excuses or ignore this stuff, as unimportant as it might seem.

Thanks for taking this into consideration, and hopefully the issue can be straightened out without too much trouble.

Note: See TracTickets for help on using tickets.