fix(server):inconsistent use of 'use strict' #2537

TDA · 2015-06-05T23:09:52Z

Fixes #1318
This fixes the files that use 'use strict' inside function scope and puts all the 'use strict' directives in the global scope.
This excludes files in the following directories:
.tmp
node_modules
app/bower_components
app/scripts/vendor
dist

@zaach

vladikoff · 2015-06-05T23:12:05Z

Gruntfile.js

@@ -1,3 +1,5 @@
+'use strict';
+
 #!/usr/bin/env node


This must on the first line

Oops, will do!

vladikoff · 2015-06-05T23:15:20Z

the 3 comments I left apply to all changes in other files

TDA · 2015-06-07T19:49:29Z

@vladikoff I think it should be right now. Fixed those newlines up.

vladikoff · 2015-06-07T21:12:48Z

app/scripts/head/start.js


 (function () {
-  'use strict';
-
+


extra spaces / line breaks still left in the diff

It was there before I changed anything. You want me to remove those as well?

the extra line break is due to use strict.
for example here:

if we remove use strict we don't need the extra line break anymore. Also see my comment regarding the JSCS rule.

vladikoff · 2015-06-07T21:15:36Z

The best styling + consistency should be:

a new line (empty) line after use strict.

vladikoff · 2015-06-07T21:17:36Z

Please add this rule to JSCS http://jscs.info/rule/requirePaddingNewLinesAfterUseStrict.html

TDA · 2015-06-08T03:44:28Z

@vladikoff looks good?

vladikoff · 2015-06-08T13:46:13Z

/home/travis/build/mozilla/fxa-content-server/grunttasks/l10n-create-json.js:8
const mkdirp         = require('mkdirp');
^^^^^
Loading "l10n-create-json.js" tasks...ERROR
>> SyntaxError: Use of const in strict mode.
Loading "l10n-extract.js" tasks...ERROR
>> SyntaxError: Use of const in strict mode.
Loading "l10n-merge.js" tasks...ERROR
>> SyntaxError: Use of const in strict mode.
Loading "l10n-supported-locales.js" tasks...ERROR
>> SyntaxError: Use of const in strict mode.
Loading "marked.js" tasks...ERROR
>> SyntaxError: Use of const in strict mode.
Loading "po2json.js" tasks...ERROR
>> SyntaxError: Use of const in strict mode.
Running "validate-shrinkwrap" task

vladikoff · 2015-06-08T13:53:11Z

We need to split out JSCS style rules between browser and node.js code. I think the jscs plugin supports extending rules from each other ( just like we have jshintrc files, in root and in app directory)

You have 2 options:

Option 1.

Split this pull request into 2 parts:
Part 1: the use strict basic update (what you had originally)
part 2: JSCS update to support both Node.js and Browser style rules.

Option 2.

Keep this all as a single PR but you might hit conflicts when you update it due to other pull requests.

vladikoff · 2015-06-08T19:31:44Z

All node files are throwing errors:

/home/travis/build/mozilla/fxa-content-server/grunttasks/l10n-create-json.js:8
const mkdirp         = require('mkdirp');
^^^^^
Loading "l10n-create-json.js" tasks...ERROR
>> SyntaxError: Use of const in strict mode.
Loading "l10n-extract.js" tasks...ERROR
>> SyntaxError: Use of const in strict mode.
Loading "l10n-merge.js" tasks...ERROR
>> SyntaxError: Use of const in strict mode.
Loading "l10n-supported-locales.js" tasks...ERROR
>> SyntaxError: Use of const in strict mode.
Loading "marked.js" tasks...ERROR
>> SyntaxError: Use of const in strict mode.
Loading "po2json.js" tasks...ERROR
>> SyntaxError: Use of const in strict mode.
Running "concurrent:lint" (concurrent) task

    Loading "l10n-create-json.js" tasks...ERROR
    >> SyntaxError: Use of const in strict mode.
    Loading "l10n-extract.js" tasks...ERROR
    >> SyntaxError: Use of const in strict mode.
    Loading "l10n-merge.js" tasks...ERROR
    >> SyntaxError: Use of const in strict mode.
    Loading "l10n-supported-locales.js" tasks...ERROR
    >> SyntaxError: Use of const in strict mode.
    Loading "marked.js" tasks...ERROR
    >> SyntaxError: Use of const in strict mode.
    Loading "po2json.js" tasks...ERROR
    >> SyntaxError: Use of const in strict mode.

    Running "jsonlint:app" (jsonlint) task
    >> 3 files lint free.

vladikoff · 2015-06-08T19:32:25Z

From the lint result^

@TDA run grunt lint locally to get rid of these, make sure the task completes with no errors.

vladikoff · 2015-06-08T19:35:01Z

@TDA Might help if you set "strict": false, for the main .jshintrc in the root of the project.

TDA · 2015-06-08T20:03:38Z

Ok, looking into that now.

TDA · 2015-06-08T20:11:42Z

@vladikoff
http://stackoverflow.com/questions/22603078/syntaxerror-use-of-const-in-strict-mode
http://kangax.github.io/jstests/var-let-const/

Should I replace the const with var? or do I have to add the --harmony flag? and if so, where do I add that?

TDA · 2015-06-08T20:15:48Z

Also, @vladikoff
jscs-dev/node-jscs#1106
JSCS doesnt seem to have an extension mechanism, and defining our own presets isnt in the documentation, what do you suggest?

vladikoff · 2015-06-08T20:20:20Z

Should I replace the const with var?

No, let's just disable linting of use strict in server files. Only /app files should be checked for that rule.

or do I have to add the --harmony flag? and if so, where do I add that?

We do not support that.

vladikoff · 2015-06-08T20:25:29Z

@TDA let's just have 2 .jscsrc files , one in root, other one in /app

Follow up from mozilla#2537 Fixed some indentation issues after running JSCS Linter

TDA · 2015-06-09T20:27:55Z

@vladikoff @pdehaan @zaach This is the entire o/p of running grunt jshint, funnily enough, the jshint tests pass, so I guess the error messages are from the node interpreter, and not from running the jshint linter:

$ grunt jshint

/Users/schandramouli/Downloads/fxa-local-dev/fxa-content-server/grunttasks/l10n-create-json.js:8
const mkdirp         = require('mkdirp');
^^^^^
>>
Loading "l10n-create-json.js" tasks...ERROR
>> SyntaxError: Use of const in strict mode.
Loading "l10n-extract.js" tasks...ERROR
>> SyntaxError: Use of const in strict mode.
Loading "l10n-merge.js" tasks...ERROR
>> SyntaxError: Use of const in strict mode.
Loading "l10n-supported-locales.js" tasks...ERROR
>> SyntaxError: Use of const in strict mode.
Loading "marked.js" tasks...ERROR
>> SyntaxError: Use of const in strict mode.
Loading "po2json.js" tasks...ERROR
>> SyntaxError: Use of const in strict mode.

Running "jshint:grunt" (jshint) task

✔ No problems


Running "jshint:app" (jshint) task

✔ No problems


Running "jshint:tests" (jshint) task

✔ No problems


Running "jshint:server" (jshint) task

✔ No problems


Done, without errors.


Execution Time (2015-06-09 20:23:10 UTC)
loading tasks   1.9s  ▇▇▇▇▇▇▇▇▇▇▇▇▇▇ 30%
jshint:grunt   161ms  ▇▇ 3%
jshint:app      3.3s  ▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇ 52%
jshint:tests   798ms  ▇▇▇▇▇▇ 13%
jshint:server  183ms  ▇▇ 3%
Total 6.3s

I am assuming the way to fix them is to either move the use strict directives for these 6 files inside function scope, or avoid using const.

pdehaan · 2015-06-09T20:46:57Z

@TDA: I am assuming the way to fix them is to either move the use strict directives for these 6 files inside function scope, or avoid using const.

My vote is to probably swap from const to var in those six files to retain consistency. Otherwise we may have to do some inline JSHint rule to disable the global strict rule in those specific files (unless that is being enforced via jscs, in which case it may be trickier).

But I'll leave the whole const vs var decision up to @shane-tomlinson, @zaach, and @vladikoff.

GitCop · 2015-06-09T22:52:38Z

There were the following issues with your Pull Request

Commit: 3645ab5
- Your commit message body contains a line that is longer than 100 characters
- Commits must be in the following format: %{type}(%{scope}): %{description}

Guidelines are available at https://github.com/mozilla/fxa-content-server/blob/master/CONTRIBUTING.md#git-commit-guidelines

This message was auto-generated by https://gitcop.com

GitCop · 2015-06-09T23:01:36Z

There were the following issues with your Pull Request

Commit: 110f2b7
- Commits must be in the following format: %{type}(%{scope}): %{description}

Guidelines are available at https://github.com/mozilla/fxa-content-server/blob/master/CONTRIBUTING.md#git-commit-guidelines

This message was auto-generated by https://gitcop.com

pdehaan · 2015-06-09T23:51:55Z

    Warning: Running "jscs:server" (jscs) task
    >> 154 files without code style errors.

    Running "jscs:app" (jscs) task
    Warning: Unsupported rules: requirePaddingNewLinesAfterUseStrict� Use --force to continue.

    Aborted due to warnings.


    Execution Time (2015-06-09 23:48:10 UTC)
    loading tasks  3.8s  ▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇ 72%
    jscs:server    1.5s  ▇▇▇▇▇▇▇▇▇▇▇▇▇▇ 28%
    Total 5.3s� Use --force to continue.

        Aborted due to warnings.

Execution Time (2015-06-09 23:48:07 UTC)
loading tasks    1.7s  ▇▇▇▇▇▇▇▇▇▇ 22%
concurrent:lint  6.1s  ▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇ 78%
Total 7.9s
The command "grunt lint" exited with 6.

— https://travis-ci.org/mozilla/fxa-content-server/builds/66135872#L547

TDA · 2015-06-10T04:01:16Z

This should be because of the version of JSCS installed, but I did change the dependency to include the latest version of grunt-jscs(1.8.0), which would include the latest jscs(1.13.1) as a sub-dependency.
The lint passes on my local-dev. Not sure why it wouldn't pass when the dependencies are properly specified.
@pdehaan what do you think could be wrong?
@vladikoff anything I am missing?

pdehaan · 2015-06-10T06:19:17Z

I'm not sure what's wrong with this.
I was able to do a $ git clone of your branch locally and run grunt jscs without any errors.
Not sure if Travis is caching something and we need to flush it.

@vladikoff @shane-tomlinson @zaach Is there an easy way to see what versions of the modules that Travis has installed/cached, or should we add $ npm ls --depth 0 to the travis.yml file?

shane-tomlinson · 2015-06-10T08:06:20Z

Not sure if Travis is caching something and we need to flush it.

@pdehaan - I just went into travis and deleted the cache for the branch, if that doesn't fix the problem, I'll kill all caches.

pdehaan · 2015-06-10T16:53:07Z

@TDA I see that the Travis build is re-re-re-starting. If this build doesn't pass, maybe try putting this change in the .travis.yml file so we can see exactly which modules+versions are getting installed on Travis: https://github.com/mozilla/fxa-content-server/pull/2564/files

pdehaan · 2015-06-10T16:54:26Z

@vladikoff @shane-tomlinson Travis seems to be barfing all over the place this morning. Not sure why this PR's travis log is showing stuff like this during npm install:

$ travis_retry npm install --silent
module.js:340
    throw err;
          ^
Error: Cannot find module '../../object/valid-value'
    at Function.Module._resolveFilename (module.js:338:15)
    at Function.Module._load (module.js:280:25)
    at Module.require (module.js:364:17)
    at require (module.js:380:17)
    at Object.<anonymous> (/home/travis/build/mozilla/fxa-content-server/node_modules/grunt-contrib-imagemin/node_modules/imagemin/node_modules/vinyl-fs/node_modules/glob-stream/node_modules/unique-stream/node_modules/es6-set/node_modules/es5-ext/array/#/clear.js:7:13)
    at Module._compile (module.js:456:26)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Module.require (module.js:364:17)

Fixes mozilla#1318 Fix files that use 'use strict' in function scope. Put 'use strict' directives in the global scope. Fix spacing and newline issues.

TDA · 2015-06-10T17:09:26Z

@pdehaan Doing this now :)

pdehaan · 2015-06-10T17:12:50Z

Doing this now :)

@TDA: Actually, this build may pass. DON'T TOUCH ANYTHING! 🤞

TDA · 2015-06-10T17:14:33Z

@pdehaan :O :O I added the - npm ls --depth 0 to the travis file, should I remove it?

TDA · 2015-06-10T17:24:18Z

@pdehaan What dark magic is this, master?

pdehaan · 2015-06-10T17:35:55Z

I think Travis is just having some issues today.
I was staring at my #2564 PR and it started failing on npm install a few times.

Let's just cross our fingers that we can get this merged before we hit merge conflicts or have to do any more pushes. 😓

TDA · 2015-06-10T17:47:18Z

Fair enough. Looks like its merged now 💃

TDA added the waffle:needs review label Jun 5, 2015

vladikoff reviewed Jun 5, 2015
View reviewed changes

TDA force-pushed the issue-1318-use-strict branch 4 times, most recently from 8dd27a8 to 2b75e68 Compare June 7, 2015 19:48

TDA force-pushed the issue-1318-use-strict branch from 2b75e68 to 6e4a323 Compare June 7, 2015 19:56

vladikoff reviewed Jun 7, 2015
View reviewed changes

TDA force-pushed the issue-1318-use-strict branch 2 times, most recently from 93f0df5 to 4e9e99f Compare June 8, 2015 03:43

TDA force-pushed the issue-1318-use-strict branch from 4e9e99f to 42dddc0 Compare June 8, 2015 18:30

vladikoff added this to the train 40 milestone Jun 8, 2015

TDA added a commit to TDA/fxa-content-server that referenced this pull request Jun 9, 2015

chore(lint): fix indentation after jscs lint

8fe1c5d

Follow up from mozilla#2537 Fixed some indentation issues after running JSCS Linter

pdehaan mentioned this pull request Jun 9, 2015

Switch from JSHint to ESLint mozilla/fxa-js-client#156

Closed

TDA added a commit to TDA/fxa-content-server that referenced this pull request Jun 9, 2015

chore(lint): fix indentation after jscs lint

face133

Follow up from mozilla#2537 Fixed some indentation issues after running JSCS Linter

TDA force-pushed the issue-1318-use-strict branch from 42dddc0 to 3645ab5 Compare June 9, 2015 22:52

TDA force-pushed the issue-1318-use-strict branch from 3645ab5 to 110f2b7 Compare June 9, 2015 23:01

TDA force-pushed the issue-1318-use-strict branch from 110f2b7 to f777b04 Compare June 9, 2015 23:05

TDA mentioned this pull request Jun 9, 2015

decide whether 'use strict' is appropriate for node code #2558

Closed

TDA force-pushed the issue-1318-use-strict branch from f777b04 to bf5ee50 Compare June 9, 2015 23:42

TDA force-pushed the issue-1318-use-strict branch from bf5ee50 to a54b4e1 Compare June 10, 2015 16:29

pdehaan mentioned this pull request Jun 10, 2015

chore(travis): add npm -ls to travis file #2564

Merged

fix(server): inconsistent use of 'use strict'

cc88e84

Fixes mozilla#1318 Fix files that use 'use strict' in function scope. Put 'use strict' directives in the global scope. Fix spacing and newline issues.

TDA force-pushed the issue-1318-use-strict branch from a54b4e1 to cc88e84 Compare June 10, 2015 17:08

TDA closed this in 38ab1c0 Jun 10, 2015

TDA removed the waffle:needs review label Jun 10, 2015

fix(server):inconsistent use of 'use strict' #2537

fix(server):inconsistent use of 'use strict' #2537

Conversation

TDA commented Jun 5, 2015

vladikoff Jun 5, 2015

Choose a reason for hiding this comment

TDA Jun 5, 2015

Choose a reason for hiding this comment

vladikoff commented Jun 5, 2015

TDA commented Jun 7, 2015

vladikoff Jun 7, 2015

Choose a reason for hiding this comment

vladikoff Jun 7, 2015

Choose a reason for hiding this comment

TDA Jun 7, 2015

Choose a reason for hiding this comment

vladikoff Jun 7, 2015

Choose a reason for hiding this comment

vladikoff commented Jun 7, 2015

vladikoff commented Jun 7, 2015

TDA commented Jun 8, 2015

vladikoff commented Jun 8, 2015

vladikoff commented Jun 8, 2015

Option 1.

Option 2.

vladikoff commented Jun 8, 2015

vladikoff commented Jun 8, 2015

vladikoff commented Jun 8, 2015

TDA commented Jun 8, 2015

TDA commented Jun 8, 2015

TDA commented Jun 8, 2015

vladikoff commented Jun 8, 2015

vladikoff commented Jun 8, 2015

TDA commented Jun 9, 2015

pdehaan commented Jun 9, 2015

GitCop commented Jun 9, 2015

GitCop commented Jun 9, 2015

pdehaan commented Jun 9, 2015

TDA commented Jun 10, 2015

pdehaan commented Jun 10, 2015

shane-tomlinson commented Jun 10, 2015

pdehaan commented Jun 10, 2015

pdehaan commented Jun 10, 2015

TDA commented Jun 10, 2015

pdehaan commented Jun 10, 2015

TDA commented Jun 10, 2015

TDA commented Jun 10, 2015

pdehaan commented Jun 10, 2015

TDA commented Jun 10, 2015