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 functionalities
Create patch and upload for the testbot.

Files: system/system.cron.js, system/system.js

CommentFileSizeAuthor
#16 core_js-jshint_system-1684872-16.patch3.08 KBpascalduez
#8 1684872_system-jshint_8.patch3.09 KBfrega
#6 system-jshint.patch3.05 KBdroplet
#5 system-jshint.patch3.19 KBdroplet
#3 system-jshint.patch3.1 KBdroplet
#2 core-js-jshint-system.patch1.46 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/modules/system/system.js: line 36, col 9, The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
core/modules/system/system.js: line 42, col 8, Don't make functions within a loop.
core/modules/system/system.js: line 31, col 5, The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
core/modules/system/system.js: line 66, col 10, Don't make functions within a loop.
nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Status: Active » Needs work
FileSize
core-js-jshint-system.patch1.46 KB

removed first half of the warnings.

droplet’s picture

droplet CreditAttribution: droplet commented
Status: Needs work » Needs review
FileSize
system-jshint.patch3.1 KB
nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Status: Needs review » Needs work

there is an issue with fieldSettings it's not going to work out like you think. When learning JS that's actually the typical case where you'll first encounter closures.

dateTimeHnalder needs to get the fieldSettings, source, suffix as parameters and return a function. I might get some time to reroll the patch tomorrow to show what I mean.

droplet’s picture

droplet CreditAttribution: droplet commented
Status: Needs work » Needs review
FileSize
system-jshint.patch3.19 KB
droplet’s picture

droplet CreditAttribution: droplet commented
FileSize
system-jshint.patch3.05 KB

remove unused var.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Status: Needs review » Needs work

the blur thing won't work, custom data needs to be sent with the event binding.

frega’s picture

frega CreditAttribution: frega commented
Status: Needs work » Needs review
FileSize
1684872_system-jshint_8.patch3.09 KB

Fixed lacking custom data. jshint passes.

Note: further improvement as copyFieldValue "targetIds" is always (i.e. the *one* time it's used: drupal_add_js(array('copyFieldValue' => array('edit-site-mail' => array('edit-account-mail'))), 'setting');)

Then we could replace "for (var delta in targetIds)" in the code (assuming looping over properties of an object) with a "proper" iteration of an array.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

Looks good to me, needs someone else to review :) I touched the patch too.

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

The last submitted patch, 1684872_system-jshint_8.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

#8: 1684872_system-jshint_8.patch queued for re-testing.

pivica’s picture

pivica CreditAttribution: pivica commented

Related to https://drupal.org/node/492720#comment-6376408, we should merge this two patches.

pascalduez’s picture

pascalduez CreditAttribution: pascalduez commented

Reviewed, tested. Looks good to me.

on line 46
$('#' + sourceId, context)
should maybe be converted to
$(context).find('#' + sourceId)

.bind('blur'
should maybe be converted to
.on('blur'

on line 64
.keyup(function ()
should maybe be converted to
.on('keyup', function()

pascalduez’s picture

pascalduez CreditAttribution: pascalduez commented
Status: Needs review » Reviewed & tested by the community
nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Status: Reviewed & tested by the community » Needs work

actually, $('#' + sourceId) would be better.

The .on change should be handled in the other issue.

pascalduez’s picture

pascalduez CreditAttribution: pascalduez commented
FileSize
core_js-jshint_system-1684872-16.patch3.08 KB

$('#' + sourceId, context) converted to $('#' + sourceId).

pivica’s picture

pivica CreditAttribution: pivica commented

Patch http://drupal.org/files/refactoring-system.js-492720.patch from #492720: Refactor system.js file and use Drupal form states and form ajax removes most of current js code from system.js, so most of this patch will not be needed. If you can please review that patch also because it is affecting this issue.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Status: Needs work » Postponed

let's wait for the split up issue to get in and check back afterwards with jshint

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Status: Postponed » Fixed

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