Follow-up of #1664940: [Policy, patch] Decide on JSHint configuration and part of #1415788: 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

CommentFileSizeAuthor
#20 core-jshint-ajax.js-1684788-20.patch550 bytesnod_
#10 core_js-jshint_ajax-1684788-9.patch7.04 KBpascalduez
#6 core_js-jshint_ajax-1684788-6.patch6.39 KBpascalduez
#5 core_js-jshint_ajax-1684788-5.patch6.39 KBpascalduez
#4 core_js-jshint_ajax-1684788-4.patch8.43 KBpascalduez
#3 core_js-jshint_ajax-1684788-3.patch5.63 KBpascalduez
#2 core-js-jshint-ajax.patch4.3 KBnod_
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented 
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

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Status: Active » Needs work
FileSize
core-js-jshint-ajax.patch4.3 KB

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

pascalduez CreditAttribution: pascalduez commented
FileSize
core_js-jshint_ajax-1684788-3.patch5.63 KB

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

pascalduez CreditAttribution: pascalduez commented
FileSize
core_js-jshint_ajax-1684788-4.patch8.43 KB

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

pascalduez CreditAttribution: pascalduez commented
FileSize
core_js-jshint_ajax-1684788-5.patch6.39 KB

Removed changes in ajax_command_css() and ajax_command_settings().

pascalduez’s picture

pascalduez CreditAttribution: pascalduez commented
FileSize
core_js-jshint_ajax-1684788-6.patch6.39 KB

Closure arguments: removed spaces, inverted order. :)

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
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

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
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

pascalduez CreditAttribution: pascalduez commented
Status: Needs review » Needs work
FileSize
core_js-jshint_ajax-1684788-9.patch7.04 KB

Corrected "arguments" change in testAjaxCommands().

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Status: Needs work » Needs review
frega’s picture

frega CreditAttribution: frega commented
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

seutje CreditAttribution: seutje commented

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

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

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

seutje’s picture

seutje CreditAttribution: seutje commented

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

Jelle_S’s picture

Jelle_S
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: Jelle_S commented

Patch passes Testswarm tests for autocomplete.

RTBC++

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
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

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
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

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Status: Needs work » Needs review
FileSize
core-jshint-ajax.js-1684788-20.patch550 bytes

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

droplet’s picture

droplet CreditAttribution: droplet commented
Status: Needs review » Reviewed & tested by the community

simple one

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
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.