Make WordPress Core

Opened 11 years ago

Closed 11 years ago

#25187 closed enhancement (fixed)

.jshintrc & .jshintignore support

Reported by: carldanley's profile carldanley Owned by:
Milestone: 3.8 Priority: normal
Severity: normal Version: 3.8
Component: Build/Test Tools Keywords: dev-feedback needs-testing
Focuses: Cc:

Description

Implement jshint configuration standards so everyone is on the same page. See my notes in this gist: https://gist.github.com/carldanley/6384499. Feel free to open discussion on any config option. Thinking we can patch that gist until we feel really confident about it and then I'll submit a formal patch when ready.

Attachments (19)

25187.diff (2.3 KB) - added by carldanley 11 years ago.
25187.2.diff (3.8 KB) - added by kadamwhite 11 years ago.
.jshintrc adapted from the jQuery project, with appropriate Grunt configuration
25187.3.diff (3.8 KB) - added by kadamwhite 11 years ago.
Added "onevar" to Gruntfile jshint options
25187.4.diff (3.9 KB) - added by kadamwhite 11 years ago.
Add "jQuery" and "wp" as globals
25187.5.diff (1.3 KB) - added by jorbin 11 years ago.
25187.6.diff (4.9 KB) - added by kadamwhite 11 years ago.
Add .jshintrc for qunit and bring existing the unit test file in line with jQuery style guide
25187.7.diff (249 bytes) - added by jorbin 11 years ago.
add ajaxurl to globals
25187.8.diff (280 bytes) - added by jorbin 11 years ago.
25187.9.diff (274 bytes) - added by jorbin 11 years ago.
25187.10.diff (1.4 KB) - added by tollmanz 11 years ago.
Implement jshinting for WP code within "3rd Party Scripts" directories
25187.11.diff (1.5 KB) - added by tollmanz 11 years ago.
Adds "wp-includes/js/tinymce/langs/wp-langs-en.js", which was missed in the last diff.
25187.11.2.diff (1.5 KB) - added by tollmanz 11 years ago.
Whoops! File path was not correct in 25187.11.diff
25187.12.diff (1.3 KB) - added by kadamwhite 11 years ago.
Suppress "curly" and "eqeqeq" errors for WP code, remove duplicate "onevar" from gruntfile jshint config, add common libs to jshintrc, and alphabetize jshintrc
25187.13.diff (1.4 KB) - added by kadamwhite 11 years ago.
Update unit test JS to pass now that "onevar" is true
25187.14.diff (2.8 KB) - added by nacin 11 years ago.
25187.15.diff (831 bytes) - added by azaozz 11 years ago.
25187.16.diff (829 bytes) - added by kadamwhite 11 years ago.
Standardize spacing and make JSHint filter pass eqeqeq
25187.17.diff (2.0 KB) - added by azaozz 11 years ago.
25187.18.diff (3.4 KB) - added by tollmanz 11 years ago.

Download all attachments as: .zip

Change History (53)

@carldanley
11 years ago

#1 @carldanley
11 years ago

25187.diff implements the new files.

#2 @jorbin
11 years ago

Do we need to be declaring every option in jshint? I think it would be easier to read the jshint file if we only included the options where we disagreed with the default.

#3 @carldanley
11 years ago

Definitely not. I put them there for starters so everyone relevant can view in the event they're unsure of what each option is. We can remove the defaults before we patch; totally agree with that.

Last edited 11 years ago by carldanley (previous) (diff)

@kadamwhite
11 years ago

.jshintrc adapted from the jQuery project, with appropriate Grunt configuration

@kadamwhite
11 years ago

Added "onevar" to Gruntfile jshint options

@kadamwhite
11 years ago

Add "jQuery" and "wp" as globals

#4 @kadamwhite
11 years ago

Added .jshintrc adapted from the jQuery project, with these modifications:

  • Prefer single quotes to double
  • Remove "boss" option
  • Add "jQuery" and "wp" as globals

We do not enforce onevar in .jshintrc, due to the number of files in which this is an issue. It can be added file-by-file as they get fixed, and rolled into the global config down the road.

gnarf37 & I did this work at the wcbos dev day after discussion with mattweibe

#5 @nacin
11 years ago

In 25960:

Add a .jshintrc file and associated jshint grunt task.

This .jshintrc is adopted from the jQuery project, with some basic modifications (such as single quotes instead of double quotes). This pretty closely follows our current JS standards and rather closely resembles our PHP standards, especially the love of whitespace. The major changes are enforcing === and always using braces for if statements.

props kadamwhite, gnarf37, with mattwiebe and carldanley.
see #25187.

@jorbin
11 years ago

#6 follow-up: @jorbin
11 years ago

the above patch makes the jshint:qunit task pass.

@kadamwhite
11 years ago

Add .jshintrc for qunit and bring existing the unit test file in line with jQuery style guide

#7 in reply to: ↑ 6 @kadamwhite
11 years ago

Replying to jorbin:

the above patch makes the jshint:qunit task pass.

Good catch! Corey actually worked on this a bit yesterday, too, but in my excitement I left Corey's QUnit .jshintrc out from the original patch. I've uploaded 25187.6.diff, which contains exactly what jorbin is suggesting and also brings the test file in line with the jQuery style guide (per our conversation with @mattwiebe, @nacin, @gnarf37, and @tommcfarlin about formally adopting most of that style guide).

#8 @jorbin
11 years ago

I do not agree with the style being used in the for loops on 25187.6. I think that they make the for loops harder to read and should not be the style we aim for. Resetting i for use in another loop is not obvious and we should avoid using the variable name length and instead be more obvious as to which length we are talking about. (yes, I may have been the one to originally use that name. I'm not above saying I did it wrong).

Can you share the rest of this meat space conversation?

@jorbin
11 years ago

add ajaxurl to globals

#9 @jorbin
11 years ago

related: #25754

#10 @nacin
11 years ago

In 25992:

Add jshintrc to qunit.

props jorbin.
see #25187.

@jorbin
11 years ago

#11 @jorbin
11 years ago

Above patch adds es3 and onevar as agreed upon in the irc chat for JS standards.

https://irclogs.wordpress.org/chanlog.php?channel=wordpress-dev&day=2013-11-06&sort=asc#m722215

#12 @kadamwhite
11 years ago

Once onevar and es3 are added, we should probably close this and move on. Per the IRC log linked above, individual files' jshint errors will be addressed incrementally in the manner being used by the inline docs effort

@jorbin
11 years ago

#13 @azaozz
11 years ago

In 26025:

Add es3 and onevar to the JSHint configuration as discussed in IRC. Props jorbin, see #25187.

#14 @tollmanz
11 years ago

I just audited the JS files in WordPress in order to prepare a list of files that we need to cleanup in order for jshint to pass. In this process, I found that a number of files that are WordPress code, not 3rd party code, are buried in the "3rd-Party Scripts" excludes. These all need to be added to the jshint task:

# Could not be positively identified as 3rd party
wp-includes/js/jquery/jquery.table-hotkeys.js
 
# WP specific tinyMCE plugins
wp-includes/js/tinymce/plugins/wordpress/editor_plugin_src.js
wp-includes/js/tinymce/plugins/wpdialogs/editor_plugin_src.js
wp-includes/js/tinymce/plugins/wpdialogs/js/popup.js
wp-includes/js/tinymce/plugins/wpdialogs/js/wpdialog.js
wp-includes/js/tinymce/plugins/wpeditimage/editor_plugin_src.js
wp-includes/js/tinymce/plugins/wpeditimage/js/editimage.js
wp-includes/js/tinymce/plugins/wpfullscreen/editor_plugin_src.js
wp-includes/js/tinymce/plugins/wpgallery/editor_plugin_src.js
wp-includes/js/tinymce/plugins/wplink/editor_plugin_src.js
wp-includes/js/tinymce/plugins/wpview/editor_plugin_src.js
 
# Other
wp-includes/js/tinymce/wp-tinymce-schema.js
wp-includes/js/mediaelement/wp-mediaelement.js
wp-includes/js/plupload/wp-plupload.js

A thorough accounting of jshint status can be found here: https://gist.github.com/tollmanz/7348654. @kadamwhite should be posting this to a Make blog tomorrow.

I found that reinstating these individual files after the directories have been excluded works the charm for getting them jshinted. If there is a better way to do this, I am all ears.

@tollmanz
11 years ago

Implement jshinting for WP code within "3rd Party Scripts" directories

#15 @jorbin
11 years ago

Based on #7643, I'm pretty sure wp-includes/js/jquery/jquery.table-hotkeys.js isn't third party.

@nbachiyski - Any chance you remember from 5+ years ago?

#16 @tollmanz
11 years ago

I absolutely couldn't find that ticket. Yes...that seems to be WP code based on that ticket. If it's not, there is a good chance that we are violating some license ;)

@jorbin - Do the changes to Gruntfile.js look good? Know of any other way to do that?

@tollmanz
11 years ago

Adds "wp-includes/js/tinymce/langs/wp-langs-en.js", which was missed in the last diff.

@tollmanz
11 years ago

Whoops! File path was not correct in 25187.11.diff

#17 @jorbin
11 years ago

@tollmanz - There are two alternatives that I see:

  1. We could use the {} globbing pattern a bit, but I think that listing the files out the way you did is more readable.
  2. We can use the method that jQuery uses of grouping files and then mapping them, but I think this only makes sense if we are going to reuse the list for other tasks.

Overall, I think your method makes the most sense for now.

#18 @tollmanz
11 years ago

Thanks @jorbin! I agree. This method seems to be the most sensible.

@kadamwhite
11 years ago

Suppress "curly" and "eqeqeq" errors for WP code, remove duplicate "onevar" from gruntfile jshint config, add common libs to jshintrc, and alphabetize jshintrc

@kadamwhite
11 years ago

Update unit test JS to pass now that "onevar" is true

@nacin
11 years ago

#19 @kadamwhite
11 years ago

Added two patches: One to adjust our global JSHint options,

  • Add Backbone and _ as universally permitted global variables
  • Set "curly" and "eqeqeq" to FALSE within the Gruntfile's jshint configuration for wp-admin/ and wp-includes/, to aid in addressing the code smell first
  • Alphabetize jshintrc options for readability

and the other to make the unit test JSHint run pass now that "onevar" is true

#20 @nacin
11 years ago

In 26040:

Use only one var per scope in our password-strength-meter tests.

props kadamwhite.
see #25187.

#21 @nacin
11 years ago

In 26041:

Update jshint grunt task to exclude eqeqeq and curly for now, until we've resolved other code smells.

props kadamwhite.
see #25187.

#22 @nacin
11 years ago

In 26042:

Ignore wp-langs-en.js during linting, per azaozz. see #25187.

@azaozz
11 years ago

#23 follow-up: @azaozz
11 years ago

In 25187.15.diff: make it possible to process only one file passed from cli:

$ jslint:core --target=filename.js

@kadamwhite
11 years ago

Standardize spacing and make JSHint filter pass eqeqeq

#24 @kadamwhite
11 years ago

In 25187.16.diff, made these adjustments to azaozz's jshint filter:

  • Fixed a jshint error within the jshint grunt task filter method :D
  • Made the spacing consistent
  • Changed "target" to "file," since that's what we mean

Can be run by

grunt jshint:core --target=xfn.js

#25 in reply to: ↑ 23 @adamsilverstein
11 years ago

Replying to azaozz:

In 25187.15.diff: make it possible to process only one file passed from cli:

$ jslint:core --target=filename.js

brilliant! this will really help getting thru the files one by one, thanks!

#26 @azaozz
11 years ago

In 26043:

Grunt: limit JSHint's run to a single specified file. Run with grunt jshint:core --file=filename.js. Props kadamwhite, see #25187.

#27 @azaozz
11 years ago

We can also take that filter function outside the jshint:core task and add it to all of jshint. Perhaps a bit easier to run: grunt jshint --file=about.js, but will output a few more lines:

$ grunt jshint --file=about.js
Running "jshint:grunt" (jshint) task
>> 0 files linted. Please check your ignored files.

Running "jshint:tests" (jshint) task
>> 0 files linted. Please check your ignored files.

Running "jshint:core" (jshint) task
Linting src/wp-admin/js/about.js ...ERROR
...
Last edited 11 years ago by azaozz (previous) (diff)

@azaozz
11 years ago

#28 @kadamwhite
11 years ago

+1 on generalizing the filter code. This would be particularly useful when working on unit tests.

#29 @azaozz
11 years ago

Yeah, as the filtering is done while grunt selects files to process, this function (with some tweaks) can be used in any task to limit to one file or directory or to exclude a file or directory. Can also be made to accept multiple files/directories from the cli if needed.

@tollmanz
11 years ago

#30 @tollmanz
11 years ago

25187.18.diff is a first pass at the merge of the sticky menu component into core. In the merge process, I have updated a few things:

  • I added a debounce function to the resize and scroll events. It is important that this is throttled
  • Everything was cleaned up to pass the jshint rules
  • All repeated selectors were cached

A few questions about this:

  1. Given the integration into core via add JS to an existing JS file and CSS to an existing CSS file, I removed the PHP related to this component that only served to load the assets. One piece of that PHP is that it did not load this JS/CSS on mobile other than iPad. I could not figure out why this was a requirement. Is this needed? Obviously, where possible, especially with a project like WordPress, browser sniffing is not desirable, so it would be nice to remove this altogether, but I do not have an idea as to the implications of it.
  2. I stole Underscore.js's debounce function. I did not want to include Underscore.js as it would make Underscore.js a dependency for all of WP admin. Any thoughts use of this function?
Version 0, edited 11 years ago by tollmanz (next)

#31 @tollmanz
11 years ago

Oopsie...wrong ticket. Sorry about that.

#32 @nacin
11 years ago

In 26136:

Remove smarttabs from .jshintrc per IRC discussion. see #25187.

#33 @nacin
11 years ago

In 26209:

JSHint fixes for word-count.js and wp-mediaelement.js. see #25187.

#34 @nacin
11 years ago

  • Component changed from General to Build Tools
  • Milestone changed from Awaiting Review to 3.8
  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.