Global javascript variables incorrectly defined

[johnjamesjacoby]

customize-controls.js and customize-models.js both use globals instead of global

This is causing code completion and scanning issues in PHPStorm & Netbeans in my experience.

Patch imminent to recommend using global over globals

[johnjamesjacoby]

33021.1.patch does the following:

• Uses global over globals
• Adds missing _ and Backbone global defines

[jorbin]

According to the jshint documentation, ​globals is actually the correct term. I know we are not consistant between them right now, but I don't think the code churn is worth it to clean it up.

As far as _ and Backbone, both of those are declared as globals for the entire project in .jshintrc.

[johnjamesjacoby]

According to the JSLint documentation, ​global is actually the correct term to use in the context of the .js files themselves, where as globals is what's expected in JSHint's .jshintrc file itself.

As far as _ and Backbone, you are correct that both are declared in .jshintrc, but I think they need declaring in the files they are used independently so IDE's can parse them appropriately. See also the many places where jQuery, wp, and others are redeclared in other .js files.

I don't think either of us are wrong, just that you're pointing to one reference for solving the automated testing JSHint performs, and I'm pointing to another reference for assisting developers who are writing code in IDEs with built in JSLint support (PHPStorm, Netbeans, etc...)

See also the ​WordPress core handbook page on Javascript coding standards and defining globals, where global is the suggested and recommended keyword for this purpose.

[johnjamesjacoby]

WordPress chose JSHint over JSLint, and please know that I'm not attempting to shift endorsement from one to the other. I only wish to point out and patch an inconsistency that JSLint helped identify that can easily be fixed to provide a more friendly developer experience with very little code churn by updating 2 lines of code to match the spec.

[netweb]

I'm going to endorse a third option to this discussion, eslint I plan on (and will create a new ticket) for us to switch to ESLint which will replace JSHint and JSValidate (I'll outline the benefits for us in that new ticket).

The ESLint docs here: ​http://eslint.org/docs/user-guide/configuring#specifying-globals

These docs also match up with what John has stated above, globals in the config files, and global in the .js files.

I'm +1 on this proposed change, it fixes the IDE issues, inline with our own JS Docs, and aligns with best practices.

[jorbin]

If An IDE has bugs in it's jshint implementation, then the bug report should be filed with them. Making changes to match a spec is a trip down a rabbit hole. As the ​Javscript Coding standards specifically state:

While the coding standards are important, refactoring older .js files simply to conform to the standards is not an urgent issue. “Whitespace-only” patches for older files are strongly discouraged.

I think this should be closed maybelater. When the lines are touched for a substantial reason, we should fix them up.

[nacin]

I agree with @jorbin that we should not be including globals that are listed in .jshintrc (and remove them where they are duplicated). An IDE *must* parse this file to be able to do anything effective with regards to JavaScript linting, as it needs to know the rest of the flags of the project. The globals directive is the same for JSHint and JSLint in this case.

Within the file, I agree that we should standardize on /*global*/. JSLint does not appear to support /*globals*/. Additionally, it uses /*global*/ in its own files: ​https://github.com/douglascrockford/JSLint/blob/master/browser.js.

JSHint supports both: ​https://github.com/jshint/jshint/blob/master/src/lex.js#L412. However, it uses /*global*/ in its own files: ​https://github.com/jshint/jshint/blob/master/src/jshint.js#L32. JSHint does, of note, confuses this in its documentation: ​http://jshint.com/docs/#inline-configuration. It specifies /*jshint*/ and /*global*/ but then proceeds to document it as /*globals*/. I've submitted an issue to the website: ​https://github.com/jshint/jshint.github.io/issues/27.

Bottom line: IDEs should be responsible for parsing .jshintrc. I don't think they need to take responsibility for parsing an underused alias.

[nacin]

In 33324:

JSHint: Use 'global' rather than 'globals' for inline configuration.

Don't declare a global already listed in .jshintrc.

props johnjamesjacoby.
fixes #33021.

[johnjamesjacoby]

More documentation for PHPStorm & Netbeans users:

• JetBrains/PHPStorm: ​https://www.jetbrains.com/phpstorm/help/jshint.html
• Netbeans (via plugin): ​http://plugins.netbeans.org/plugin/58580/jshint

Jorbin/Nacin:

• Do either of you use PHPStorm or Netbeans?
• If so, have you configured this to better comprehend the benefits?
• If not, in the future can we delegate decisions like these to someone who can speak to the importance of their effectiveness before threatening to close the next ticket(s) that come in?

[nacin]

Replying to johnjamesjacoby:

Jorbin/Nacin:

• Do either of you use PHPStorm or Netbeans?
• If so, have you configured this to better comprehend the benefits?
• If not, in the future can we delegate decisions like these to someone who can speak to the importance of their effectiveness before threatening to close the next ticket(s) that come in?

Great questions. I suspect @jorbin just misunderstood the significance of global versus globals. It's confusing — I even had to submit a bug report, because the JSHint website is confused too! Beyond that, the .jshintrc file absolutely needs to be read by the IDE for any JSHint integration to be useful, so it made total sense to mark this ticket for closure. As your links indicated, both PHPStorm and Netbeans read from .jshintrc, so there you go. I am not sure if you think there are additional benefits here that deserve an additional commit?

I have used PHPStorm in the past, and I just finished managing a project to integrate JSHint into a project with more JavaScript than WordPress; it's a Java project and thus this involved JSHint + Eclipse. So yeah, some experience here.

[iseulde]

In 33337:

JSHint: don't redeclare globals listed in .jshintrc

See #33021.