#1415788: Javascript winter clean-up

In a number of places in core js there are sloppy checks made against undefined or null which are not testing what it was meant to.

CommentFileSizeAuthor
#24 overlay_fix.patch599 bytesdroplet
#23 core-js-strict-eq-1428534-23.patch61.03 KBnod_
#17 core-js-strict-eq-1428534-17.patch54.35 KBnod_
#15 core-js-strict-eq-1428534-14.patch44.26 KBnod_
#13 core-js-strict-eq-1428534-13.patch39.2 KBnod_
#3 core-js-strict-eq-1428534-3.patch53.08 KBnod_
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Issue tags: +JavaScript clean-up

tag

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Title: Use scrict equals » Use strict equals
nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Status: Active » Needs review
Issue tags: +Needs manual testing
FileSize
core-js-strict-eq-1428534-3.patch53.08 KB

Here it is,

There were minor issues with cookie values and the progressbar, other than that I think I went trough everything and still working fine.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Title: Use strict equals » Use type-agnostic conditions in JavaScript
Status: Needs review » Needs work 
+++ b/core/misc/ajax.js
@@ -24,7 +24,7 @@ Drupal.behaviors.AJAX = {
-        if (typeof element_settings.selector == 'undefined') {
+        if (typeof element_settings.selector === 'undefined') {

Wouldn't it make more sense to globally introduce undefined as a undefined variable in our closures?

I.e.:

(function ($, undefined) {
  ...
})(jQuery)

That would make more sense to me, since, AFAIK, that's a JS best practice either way. Separate issue, of course.

+++ b/core/misc/ajax.js
@@ -359,7 +359,7 @@ Drupal.ajax.prototype.beforeSend = function (xmlhttprequest, options) {
-  if (this.progress.type == 'bar') {
+  if (this.progress.type === 'bar') {

@@ -371,7 +371,7 @@ Drupal.ajax.prototype.beforeSend = function (xmlhttprequest, options) {
-  else if (this.progress.type == 'throbber') {
+  else if (this.progress.type === 'throbber') {

I don't really see how a type-agnostic comparison makes the code more robust or more valid in this and other cases.

Drupal.ajax.progress.type defaults to a string and is expected to be a string. If you pass something else (i.e., a different data type), then your code is broken. In general, we don't babysit broken code.

nod_’s picture

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

It could be useful to introduce undefined, on the other hand with typeof you can't have any surprise and you don't have to introduce anything to foolproof your code. I don't feel comfortable checking against undefined as a variable.

It doesn't make the code more robust or valid, it makes it consistent. It's not about babysitting, it's about maintainability. I don't want to have to think about stupid mistakes, following strict code standards removes a whole class of bugs.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Title: Use type-agnostic conditions in JavaScript » Use === and !==
droplet’s picture

droplet CreditAttribution: droplet commented
Status: Needs review » Needs work 
+++ b/core/misc/batch.jsundefined
@@ -10,7 +10,7 @@ Drupal.behaviors.batch = {
-        if (progress == 100) {
+        if (progress === '100') {

do you sure 'progress' is a string, not the number.

nod_’s picture

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

indeed, it's explicitly formated as a string on the php side.

http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/inclu...

pounard’s picture

pounard
Primary language French
CreditAttribution: pounard commented

Which is weird, PHP should probably return the rightful types?

EDIT: Some other problems may happen, such as a float casted as int or getting out of language granularity returning 100 while it's 99.999999999999999999999999 which could break a very long batch to break before the end, got this kind of errors in some use cases. The right way to fix that is to introduce a boolean "finished" into the server response and test that. While I agree in this particular case sending it as a string would do the trick, it doesn't help for people that tries to understand the code by reading it (it's not explicit). This may need another issue for this specific behavior.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented

I already suggested to introduce a finished boolean in a another issue. This js code is definitely sloppy.

pounard’s picture

pounard
Primary language French
CreditAttribution: pounard commented

Oh good to ear that.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

Issue number?

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
FileSize
core-js-strict-eq-1428534-13.patch39.2 KB

Anyhow, this is out of scope.

Reroll.

Status: Needs review » Needs work

The last submitted patch, core-js-strict-eq-1428534-13.patch, failed testing.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Status: Needs work » Needs review
FileSize
core-js-strict-eq-1428534-14.patch44.26 KB

Forgot about ajax.js somehow.

Also, test failures?

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Issue tags: +js-novice

tag + probably need reroll

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
FileSize
core-js-strict-eq-1428534-17.patch54.35 KB

reroll I missed a few files, that should be all of them.

droplet’s picture

droplet CreditAttribution: droplet commented

Code part looks fine.

nod_’s picture

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

Seriously, this needs to get in, two months without a review (thanks droplet!) is just too long for a clean-up patch like this.

I've tested things thoroughly while making the patch as #8 can attest. It all works fine.

I'm worried we'll have conflicts between patches if this gets too long to get in. There is a lot of work that needs to be done on JS, I could live with not having to reroll big patches like this one.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks!

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

wow! thanks a lot Dries.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna commented

Is this backportable or is D7 being left as-is with these logic glitches?

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Version: 8.x-dev » 7.x-dev
Status: Fixed » Needs work
FileSize
core-js-strict-eq-1428534-23.patch61.03 KB

You tell me :)

So the current patch breaks overlay and need work. It was a bit of a PITA to backport because of selectors changes bewteen D8/7. At least this patch will apply and spare quite some backport time.

Needs a full testing of everything for D7 though, we should assume it's totally different from D8.

droplet’s picture

droplet CreditAttribution: droplet commented
Version: 7.x-dev » 8.x-dev
Status: Needs work » Needs review
FileSize
overlay_fix.patch599 bytes

Overlay is borken.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Priority: Normal » Critical
Status: Needs review » Reviewed & tested by the community

This fixes overlay

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Version: 8.x-dev » 7.x-dev
Priority: Critical » Normal
Status: Reviewed & tested by the community » Patch (to be ported)

Thanks for the fix! Committed and pushed to 8.x. Moving back to 7.x since that's where it was, but I don't really think that this is a good idea for D7, as demonstrated by the fact that we just broke major user-facing functionality in 8.x without knowing it. :P

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

Agreed I think it's time D8 moves on to speed things up. A patch sharing the same fate: #1419968: Replace $('selector', domelement) with $(domelement).find('selector').

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Version: 7.x-dev » 8.x-dev
Status: Patch (to be ported) » Fixed

Great. Kicking back to 8.x and fixed.

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

Anonymous’s picture

(not verified) CreditAttribution: commented
Issue summary: View changes

remove jsperf link