Follow-up of #1664940: [Policy, patch] Decide on JSHint configuration and part of #1415788: [Meta] javascript winter clean-up

Run jshint on the files with the configuration from the parent issue or use jshint.com with the following options:

/*jshint forin:true, noarg:true, eqeqeq:true, undef:true, curly:true, browser:true, expr:true, latedef:true, newcap:true, trailing:true */
/*global Drupal, jQuery */

Fix any warnings or errors the tool finds.
Check manually that the fixes did not break any functionality.
Create patch and upload for the testbot.

Files: ajax.js

Files: 
CommentFileSizeAuthor
#20 core-jshint-ajax.js-1684788-20.patch550 bytesnod_
PASSED: [[SimpleTest]]: [MySQL] 55,799 pass(es).
[ View ]
#10 core_js-jshint_ajax-1684788-9.patch7.04 KBpascalduez
PASSED: [[SimpleTest]]: [MySQL] 40,343 pass(es).
[ View ]
#6 core_js-jshint_ajax-1684788-6.patch6.39 KBpascalduez
FAILED: [[SimpleTest]]: [MySQL] 40,365 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#5 core_js-jshint_ajax-1684788-5.patch6.39 KBpascalduez
FAILED: [[SimpleTest]]: [MySQL] 40,237 pass(es), 2 fail(s), and 3 exception(s).
[ View ]
#4 core_js-jshint_ajax-1684788-4.patch8.43 KBpascalduez
FAILED: [[SimpleTest]]: [MySQL] 40,243 pass(es), 3 fail(s), and 3 exception(s).
[ View ]
#3 core_js-jshint_ajax-1684788-3.patch5.63 KBpascalduez
FAILED: [[SimpleTest]]: [MySQL] 40,240 pass(es), 1 fail(s), and 3 exception(s).
[ View ]
#2 core-js-jshint-ajax.patch4.3 KBnod_
FAILED: [[SimpleTest]]: [MySQL] 40,250 pass(es), 1 fail(s), and 3 exception(s).
[ View ]

Comments

nod_’s picture

core/misc/ajax.js: line 33, col 8, Don't make functions within a loop.
core/misc/ajax.js: line 25, col 5, The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
core/misc/ajax.js: line 300, col 3, The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
core/misc/ajax.js: line 303, col 16, 'key' is already defined.
core/misc/ajax.js: line 303, col 3, The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
core/misc/ajax.js: line 392, col 21, ['command'] is better written in dot notation.
core/misc/ajax.js: line 392, col 61, ['command'] is better written in dot notation.
core/misc/ajax.js: line 393, col 33, ['command'] is better written in dot notation.
core/misc/ajax.js: line 534, col 20, 'settings' is already defined.
core/misc/ajax.js: line 598, col 56, Expected an identifier and instead saw 'arguments' (a reserved word).
core/misc/ajax.js: line 628, col 20, Expected a conditional expression and instead saw an assignment.
core/misc/ajax.js: line 255, col 5, 'alert' is not defined.
core/misc/ajax.js: line 444, col 3, 'alert' is not defined.
core/misc/ajax.js: line 564, col 5, 'alert' is not defined.
nod_’s picture

Status:Active» Needs work
StatusFileSize
new4.3 KB
FAILED: [[SimpleTest]]: [MySQL] 40,250 pass(es), 1 fail(s), and 3 exception(s).
[ View ]

got rid of the command, key, filtered ifs, settings, conditional expression.

Left with alert which should be fixed by #1606362: Use throw when an ajax error occurs and extend JavaScript's Error object/#1232416: Drupal alerts "An AJAX HTTP request terminated abnormally" during normal site operation, confusing site visitors/editors.
And a couple of make a function within a loop warnings.

pascalduez’s picture

StatusFileSize
new5.63 KB
FAILED: [[SimpleTest]]: [MySQL] 40,240 pass(es), 1 fail(s), and 3 exception(s).
[ View ]

Hey,

tried to fix the latest warnings.
Used window.alert as a temporary solution, waiting for #1606362: Use throw when an ajax error occurs and extend JavaScript's Error object ?

Created a new function to be used into the behaviors loading loop.

Renamed response.arguments into response["arguments"] to circumvent the reserved word issue, without changing all the arrays keys down into ajax.inc. Okay ?

pascalduez’s picture

StatusFileSize
new8.43 KB
FAILED: [[SimpleTest]]: [MySQL] 40,243 pass(es), 3 fail(s), and 3 exception(s).
[ View ]

Corrected the details we talked about.

Formatting, no space between functions args.
Named function declaration.
Changed "arguments" (which is a JavaScript reserved word into "args" in ajax_command_invoke()
Changes "argument" (for consistency) into "arg" in ajax_command_css() and ajax_command_settings().

pascalduez’s picture

StatusFileSize
new6.39 KB
FAILED: [[SimpleTest]]: [MySQL] 40,237 pass(es), 2 fail(s), and 3 exception(s).
[ View ]

Removed changes in ajax_command_css() and ajax_command_settings().

pascalduez’s picture

StatusFileSize
new6.39 KB
FAILED: [[SimpleTest]]: [MySQL] 40,365 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Closure arguments: removed spaces, inverted order. :)

nod_’s picture

Status:Needs work» Reviewed & tested by the community

works good, thanks :)

Status:Reviewed & tested by the community» Needs work
Issue tags:-JavaScript clean-up

The last submitted patch, core_js-jshint_ajax-1684788-6.patch, failed testing.

nod_’s picture

Status:Needs work» Needs review
Issue tags:+JavaScript clean-up

#6: core_js-jshint_ajax-1684788-6.patch queued for re-testing.

pascalduez’s picture

Status:Needs review» Needs work
StatusFileSize
new7.04 KB
PASSED: [[SimpleTest]]: [MySQL] 40,343 pass(es).
[ View ]

Corrected "arguments" change in testAjaxCommands().

nod_’s picture

Status:Needs work» Needs review
frega’s picture

Status:Needs review» Reviewed & tested by the community

jshint doesnt complain anymore. did a close reading review of the patch. basic functionality (checking ajax behaviors, via "tags" field in article node bundle). couldn't do a test of the beforeSerialize-bit because I couldn't find it being used in core it self. If you have a pointer, _nod, I'll do that.

seutje’s picture

Yeah, I don't think core uses ajax forms anywhere. But the code didn't really change, just some variable names and syntax. The only real change is the hasOwnProperty which actually makes it more robust for when 3rd party scripts screw with base prototypes.

nod_’s picture

It does, manage display options for fields, autocomplete, and some other places :)

seutje’s picture

oh ok, did a manual test to make sure, nothing broke, as expected.

Jelle_S’s picture

Patch passes Testswarm tests for autocomplete.

RTBC++

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

In case you, like me, were wondering why we abbreviate "arguments" to "args" which we usually do not do, #4 points out that "arguments" is a reserved keyword.

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

nod_’s picture

Status:Closed (fixed)» Needs work

New JSHint config #1995996: Update JSHint configuration.

core/misc/ajax.js: line 87, col 78, 'message' is defined but never used.
nod_’s picture

Status:Needs work» Needs review
StatusFileSize
new550 bytes
PASSED: [[SimpleTest]]: [MySQL] 55,799 pass(es).
[ View ]

it's using this.messages instead of messages now.

droplet’s picture

Status:Needs review» Reviewed & tested by the community

simple one

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed dcfda14 and pushed to 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.