#33021 closed defect (bug) (fixed)
Global javascript variables incorrectly defined
Reported by: | johnjamesjacoby | Owned by: | nacin |
---|---|---|---|
Milestone: | 4.3 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Build/Test Tools | Keywords: | has-patch |
Focuses: | javascript | Cc: |
Description
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
Attachments (2)
Change History (13)
#3
@
9 years ago
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.
#4
@
9 years ago
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.
#5
@
9 years ago
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.
#6
@
9 years ago
- Keywords close added
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.
#7
@
9 years ago
- Keywords close removed
- Milestone changed from Awaiting Review to 4.3
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.
#8
@
9 years ago
- Owner set to nacin
- Resolution set to fixed
- Status changed from new to closed
In 33324:
#9
follow-up:
↓ 10
@
9 years ago
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?
#10
in reply to:
↑ 9
@
9 years ago
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.
33021.1.patch does the following:
global
overglobals
_
andBackbone
global defines