That is needed for clean JS, details here: https://developer.mozilla.org/en/JavaScript/Strict_mode

I can say with confidence that leaking global vars are no more thanks to "use strict", it would throw an error otherwise.

Can someone point me to a place where the ajax_command_invoke is used please?

Testing

Loading pages with JS and making sure tableheader works is enough testing. Extensive manual testing is not needed. What strict mode changes is well documented and we don't use any of it beside eval in tableheader.

All is working fine for me.

CommentFileSizeAuthor
#32 core-js-use-strict-1481560-32.patch1.46 KBnod_
#22 core-js-use-strict-1481560-22.patch15.09 KBnod_
#12 core-js-use-strict-1481560-11.patch14.99 KBnod_
#3 core-js-use-strict-1481560-3.patch16.11 KBnod_
core-js-use-strict.patch16.01 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

forgot tag

Status: Needs review » Needs work

The last submitted patch, core-js-use-strict.patch, failed testing.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Status: Needs work » Needs review
FileSize
core-js-use-strict-1481560-3.patch16.11 KB
kristiaanvandeneynde’s picture

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

There are a lot more gotchas to using JavaScript in strict mode than just having to have your variables declared.

There happens to be a very recent article by Nicholas C. Zakas on this, with interesting comments from Paul Irish and Mathias Bynens.
http://www.nczonline.net/blog/2012/03/13/its-time-to-start-using-javascr...

I'm not against using "use strict"; in Drupal, but this should be thoroughly reviewed before going live. If not, we might break some JavaScript that gets dynamically added to the strict scopes or that fails other parts of the strict mode (like the 'this'-coercion).

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

I did test the patch and it worked for me. I had to fix a few vars, nothing else beside that. The JS is pretty good code to begin with.

And because of file-level closures it's only the code patched that is evaluated in strict mode. I got rid of eval in core files and we are not using anything that is changed by the strict mode (it's for future proofing).

I don't understand what you mean by "dynamically added to the strict scopes", strict is for the current scope, and the way our closures works you can't add stuff to those scopes. I can be wrong but I'd like to see how :)

kristiaanvandeneynde’s picture

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

I was referring to exposed objects (like window.jQuery & window.Drupal) that are used within strict scopes. In the event of a module manipulating them, could it break core JS as well?

I'm all for using strict mode, but before this is committed, perhaps we should look at all the things it affects and how that would impact the current JavaScript in /core/misc?

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

Drupal & jQuery are defined in the global scope, which is not strict.

The strict mode is at the function level, the following works just fine:

Drupal.strict = function () {
  "use strict";
  alert("I'm strict");
};

Drupal.notStrict = function () {
  alert("I'm a hippie");
};
kristiaanvandeneynde’s picture

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

I'm talking about the typical jQuery IIFE:

(function ($) {
  // Any code here has access to the global jQuery object through
  // either jQuery, window.jQuery or $. As a result, any changes made
  // to jQuery outside of this IIFE could impact code inside this scope.
})(jQuery);

Same goes for Drupal.settings and any other global that is used by Drupal within a jQuery IIFE.
I'm not saying it will break code, I'm asking if it can. :)

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

I won't break and I'm having a hard time seeing how you could break it. What matters if what the definition of the function is. You can use sloppy functions in strict mode, they will execute with the sloppy context. This works:

function toto (w) {
  with (w) {
    whatever = "I won't crash";
  }
};

(function () {
"use strict";
toto(window);
}());

Contrib, jQuery and any other 3rd party can be as sloppy as they want. Drupal.settings is not defined in strict mode and it only contains json, which is only text. So unless I missed something really big there won't be unwanted side-effects.

kristiaanvandeneynde’s picture

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

All right, cool. That's the kind of reassurance I needed.
Patch looks good to me.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Title: "use strict"; » Add "use strict" to all core JavaScript to enforce clean code
Status: Needs review » Reviewed & tested by the community

Makes sense to me.

Official specification: http://www.ecma262-5.com/ELS5_Annex_C.htm

A few human-readable effects: http://stackoverflow.com/a/4799455

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
FileSize
core-js-use-strict-1481560-11.patch14.99 KB

reroll because a $(this).attr('id') changed into this.id and confused got the patching all confused.

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

The last submitted patch, core-js-use-strict-1481560-11.patch, failed testing.

nod_’s picture

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

#12: core-js-use-strict-1481560-11.patch queued for re-testing.

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

The last submitted patch, core-js-use-strict-1481560-11.patch, failed testing.

nod_’s picture

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

oh yeah :)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Issue tags: +Coding standards

Tagging coding standards for Jennifer, since it seems related.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

So... It does seem like a good idea to put "use strict" in all the JavaScript files. But since our automated tests only do PHP at this time, I think we would need to do some extensive manual testing before I would say that nothing is broken by this patch. Like at least loading pages that include the JS files in several browsers, and verifying that they all compile correctly? Or better yet, making sure they still work? I don't know exactly what's necessary, and the comments above do not make that clear.

Also, I don't see any indication of exactly what testing or verification has been done for these files.

So can someone please update the issue summary with:
- In Proposed Resolution - what tests need to be done to verify this won't break anything (and maybe some justification of why this is enough)
- In Remaining Tasks - an indication that this testing has or hasn't yet been done.

Also, did someone verify that all the JS files in core now have "use strict" after this patch is applied? (assuming that they should)?

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Reviewed & tested by the community » Needs review
nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
FileSize
core-js-use-strict-1481560-22.patch15.09 KB

So we currently don't use anything in core that would be likely to break running in "strict mode" (beside the eval in tableheader, and that's still working). The only problem would be undeclared variables, there was one left that is fixed in this patch. A new file was added since #12, rerolled the patch.

Loading pages with JS and making sure tableheader works is enough testing. Extensive manual testing is not needed. What strict mode changes is well documented and we don't use any of it beside eval in tableheader. That's still working.

On my IDE putting "use strict" warn me about any issue that could happen in strict mode. So it helps during development as well as make sure we don't do stupid things in our JS. This'll to stop people from sending patches with undeclared variables.

I'll update the issue summary. If you're not convinced, let me know i'll try to be more explicit :)

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Issue summary: View changes

less optimist.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

That sounds good, as far as the testing plan.

So did someone apply the patch and load all the pages that use JS, or all the JS files, to verify that they are all working? And verify that "tableheader" (is that the table sorting header or something else?) is still working?

And by the way, the only changes I see in this patch besides adding a lot of "use strict" everywhere are in user.js (adding a variable declaration). I don't think that is tableheader, so maybe that could use some more detailed testing too?

One more thing. Why is the "use strict" placed inside the $() { } instead of at the top of the file? Do all the JS files we have start with $() {}? Excuse my ignorance... I do *some* JS but it's not my main strength...

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

Explaining is no problem :)

So yes, all JS is wrapped in (function ($) {})(jQuery); – it's in the d.o doc – so we can rely on that.

The strict mode is applied to the current scope (the current scope is whatever parent function () {} the code you're looking at is in). "use strict" is inside this, otherwise the strict mode would be global, and you don't want that. People are using sloppy JS code from jQuery plugins, themes and all that would crash in strict mode.

tableheader is what the file making sticky table header is called. Scrolling on the module page works well for testing this.

I added the var because in strict mode this would throw an exception a few lines later. I'm not changing the way this behave, I'm just making sure it's declared before using it.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Reviewed & tested by the community

This is a D8-only patch. Basic manual testing has been performed. If something was missed, we can easily fix that in a follow-up.

I consider this change similar to updating to a new jQuery version. Instead of delaying the change almost forever on manual testing upfront, it is safe to get the change in first and fix whatever comes up later. At least at this point in the release cycle.

This will need a change notice and a update to the JavaScript coding standards handbook page.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Really? I didn't see anyone indicating they had done basic manual testing on all the files. And I wasn't aware that our policy in D8 was "patch first, ask questions later"?

I'll leave this for catch/Dries/webchick to deal with. Over my head. :)

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Issue tags: +Needs manual testing

With jQuery updates specifically, I've been letting those go in without extensive cross-browser testing because with no automated test coverage (well there is some now at branch level via testswarm I think, although it's not centralized on d.o yet) it feels better to get things tested in HEAD and sort out any issues after the fact. I'd consider being on the latest stable jQuery version an absolute blocker to an 8.0 release, and there's still plenty of time and other js patches which do require manual testing to find issues. However that's an external library rather than our own code, and it seems worth this getting a once over before commit if someone will do it - so adding the tag.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active

Well that didn't happen, so to keep things going with the js cleanup I'm going to go ahead and commit it, if it does cause bugs, then it'll be showing up an existing bug rather than actually adding a new one. Will need a change notification.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Issue summary: View changes

testing protocol

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Title: Add "use strict" to all core JavaScript to enforce clean code » Change notification for: Add "use strict" to all core JavaScript to enforce clean code
nod_’s picture

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

here it is: https://drupal.org/node/1570578

Tor Arne Thune’s picture

Tor Arne Thune CreditAttribution: Tor Arne Thune commented
Title: Change notification for: Add "use strict" to all core JavaScript to enforce clean code » Add "use strict" to all core JavaScript to enforce clean code
Priority: Critical » Normal
nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Status: Fixed » Needs review
FileSize
core-js-use-strict-1481560-32.patch1.46 KB

Follow up, three variables were not properly declared in preview.js and openid.js (this is a strict mode violation).

nod_’s picture

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

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Thanks!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Thanks. Committed/pushed to 8.x.

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

Anonymous’s picture

(not verified) CreditAttribution: commented

Updated issue summary.