Webform ESLint errors

I don't object to applying this to the 7.x-4.x as well, as I think almost all these changes are at worst neutral and most are an improvement.

That said, the change to indenting the ($) closure makes this patch impossible to evaluate. I suggest that we break this into a number of small commits, each addressing one (or maybe a few) style changes. Further, any functional changes need extensive testing (such as changing == to ===).

Slight aside: my experience with === is that it only works when 100% of the code in your environment is also written to this standard. For example, you get some data that deep somewhere else was read from the database. So your numeric data is in a string. That === now creates a bug where there was none before with ==.

It's true for ===, turns out in core it was a lot less problematic than expected. We had one issue with the progress bar because the backend was returning a string instead of an integer but nothing much beside that.

That said it's possible to do several checks if the type is unknown then it's clear what's expected just reading the code. The whole concept is to make things explicit.

About the indenting and whitespace formatting it's possible to configure PHPStorm or something to autoformat the whitespace. Would an interdiff between the formated whitespace and code change would be enough to review? that way you can convert everything in one go (beside the === maybe) and still review things.

Reroll for 7.x

Added a diff that ignore whitespace changes so that it can be reviewed.

@nod_ -- Thanks for the work. Maybe some other maintainers might weigh in as I'm conflicted.

This issue illustrates exactly what I'm talking about:

-        if (progress == 100) {
+        if (progress === '100') {

So before we had resilient code which handled either 100 or '100'. Now we have brittle code that if it's ever called with a calculated percentage will break. I'm not saying that this particular change is broken, but that it illustrates the issue.

Someone would need to test every change in webform. Right now we have good reason to believe the code works. After the "clean-up", I don't have confidence in that. I'm curious. Did Dries know that those 100-ish changes to core had actually been tested, other than the automated tests?

Let's commit the non-functional changes and leave the === issue for further testing and discussion.

In D8, the attr/prop issues doesn't exist because we know we have jQuery 1.7 minimum. Just use prop. In D7, we could make the shim again. I am not fond of turning 1 clean, understandable line with a trinary operator into 6 lines of less-clear code.

Oh yes, he sure did. At least it was very explicit in our comments #1428534-7: Use === and !==. But we're talking about the batch API specifically here, this file is used when batch API is called from PHP, with variables coming from PHP. It makes sense to check for what our PHP is documented to output. How strict we can be depends on how strict things are on the PHP side and in this case it turns out it's very strict. The fact is, this code path can only be called with progress being a string, there is no need to cater for numbers.

In the case of webform the only place I can see strict equal being an issue is for a couple of formKey conditions as it relies on configuration coming from PHP and I don't know what's in there. The rest is just a lot of string comparisons, there is no "dangerous" conditions that a strict equal might break.

For the D7 prop stuff I would agree. D7 doesn't have to conform to D8 code standards so feel free to drop that, I just did a quick reroll.

Thanks to @rteijeiro and @nod_ for the issue and the patches.

For the === changes I'd be +1 on those but only if it's very clear they won't break anything, looks like that'd only be a small subset of the proposed changes.

I'm -1 on removing the ternary operators. I'm not a big fan of them, but unless they're nested I don't think they're worth removing.

Here's a patch for review. I want to get it into other hands so we can all test it in parallel. Ignoring whitespace, it:

node-type-form.js

1. Adds use strict.

Testing. Content type definition pages still shows enabled/disabled in the vertical tab description area.

select-admin.js

1. Adds use strict.
2. Includes your fix of mistakenly setting a global 'settings'. Yikes!
3. Adds ; after the end of a function declaration assignment.
4. Replaces the two trinary calls to prop/addr with a shim.
5. Implements the shim with trinary operators that return a value. Even though I think this is less clear than a single return statement, it keeps jslint's knickers untwisted.

Testing. Select components still update their option list when dynamic options lists are selected. Options list is still readonly when a dynamic list is selected and not readonly when not dynamic.

webform-admin.js

1. Adds use strict.
2. Adds ; after the end of a function declaration assignment.
3. Inlines event handler functions. I have NO idea why local variables were used for these. Probably code evolution cruft.
4. Fixed a comment typo.
5. Includes === 'default', but not testing defaultTemplate.
6. Uses and defines the prop/attr shim as above.

Testing. In downloads, radio button is still selected when you click in a submission # textfield. When you tab in and change the value, on exit the radio button is still selected. Editing an email template and setting back to Default still works. In the included components in an e-mail, clicking and unclicking a fieldset still sets/unsets the children.

webform.js

1. Adds use strict.
2. Adds ; after the end of a function declaration assignment.
3. Uses === for testing x.length.
4. Uses === for testing 'am' and 'pm'.
5. Adjusts the existing prop/attr shim to be as above.
6. Uses === for testing $.inArray() result.

There were a few places where I left ==/!= because I could not be absolutely certain that I wasn't introducing a regression. For example, there are tests of maybe_truthy == maybe_truthy. This might for all I know be testing 1 == true, which we want to be true, whereas 1 === true would be false.

Testing. Datepicker still displays and works. I didn't test the other changes. Need to test conditional getting value of am/pm time in afternoon. Need to test knowing visibility. Need to test setting value of select/list box component.

Here's a diff with no whitespace.

Reviewing #9 I couldn't find anything that looks out of place. I'd be fine with that being committed pending your last testing of the conditionals Dan as per the end of #8. Given you the one who overhauled the conditionals I'd be fine with your sign off on that.

Ummm. No one wants to test this? If that's the case, given that I think mandating exactly-equals is a policy error, I'm inclined to drop those changes and just include the pure style changes, plus the bits of code clean-up.

Wish I had the bandwidth to test these, but manually verifying === isn't high on my list of priorities, sorry. Fine with the style changes.

I dropped all the exact [not]equals from the patch (which oddly was missing two files) and applied and committed to 7.x-4.x. Also resolved a conflict and committed to 8.x.

I'm considering this item closed. I just don't think it makes sense to waste bandwidth on changes that can only break stuff that works now.

Thanks for committing the fixes :)

Great, thanks Dan.