WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

#25187 closed enhancement (fixed)

.jshintrc & .jshintignore support

Reported by: 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 2 years ago.
25187.2.diff (3.8 KB) - added by kadamwhite 2 years ago.
.jshintrc adapted from the jQuery project, with appropriate Grunt configuration
25187.3.diff (3.8 KB) - added by kadamwhite 2 years ago.
Added "onevar" to Gruntfile jshint options
25187.4.diff (3.9 KB) - added by kadamwhite 2 years ago.
Add "jQuery" and "wp" as globals
25187.5.diff (1.3 KB) - added by jorbin 2 years ago.
25187.6.diff (4.9 KB) - added by kadamwhite 2 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 2 years ago.
add ajaxurl to globals
25187.8.diff (280 bytes) - added by jorbin 2 years ago.
25187.9.diff (274 bytes) - added by jorbin 2 years ago.
25187.10.diff (1.4 KB) - added by tollmanz 2 years ago.
Implement jshinting for WP code within "3rd Party Scripts" directories
25187.11.diff (1.5 KB) - added by tollmanz 2 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 2 years ago.
Whoops! File path was not correct in 25187.11.diff
25187.12.diff (1.3 KB) - added by kadamwhite 2 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 2 years ago.
Update unit test JS to pass now that "onevar" is true
25187.14.diff (2.8 KB) - added by nacin 2 years ago.
25187.15.diff (831 bytes) - added by azaozz 2 years ago.
25187.16.diff (829 bytes) - added by kadamwhite 2 years ago.
Standardize spacing and make JSHint filter pass eqeqeq
25187.17.diff (2.0 KB) - added by azaozz 2 years ago.
25187.18.diff (3.4 KB) - added by tollmanz 2 years ago.

Download all attachments as: .zip

Change History (53)

@carldanley
2 years ago

#1 @carldanley
2 years ago

25187.diff implements the new files.

#2 @jorbin
2 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
2 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 2 years ago by carldanley (previous) (diff)

@kadamwhite
2 years ago

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

@kadamwhite
2 years ago

Added "onevar" to Gruntfile jshint options

@kadamwhite
2 years ago

Add "jQuery" and "wp" as globals

#4 @kadamwhite
2 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
2 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
2 years ago

#6 follow-up: @jorbin
2 years ago

the above patch makes the jshint:qunit task pass.

@kadamwhite
2 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
2 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
2 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
2 years ago

add ajaxurl to globals

#9 @jorbin
2 years ago

related: #25754

#10 @nacin
2 years ago

In 25992:

Add jshintrc to qunit.

props jorbin.
see #25187.

@jorbin
2 years ago

#11 @jorbin
2 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
2 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
2 years ago

#13 @azaozz
2 years ago

In 26025:

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

#14 @tollmanz
2 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
2 years ago

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

#15 @jorbin
2 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
2 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
2 years ago

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

@tollmanz
2 years ago

Whoops! File path was not correct in 25187.11.diff

#17 @jorbin
2 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
2 years ago

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

@kadamwhite
2 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
2 years ago

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

@nacin
2 years ago

#19 @kadamwhite
2 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
2 years ago

In 26040:

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

props kadamwhite.
see #25187.

#21 @nacin
2 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
2 years ago

In 26042:

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

@azaozz
2 years ago

#23 follow-up: @azaozz
2 years ago

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

$ jslint:core --target=filename.js

@kadamwhite
2 years ago

Standardize spacing and make JSHint filter pass eqeqeq

#24 @kadamwhite
2 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
2 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
2 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
2 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 2 years ago by azaozz (previous) (diff)

@azaozz
2 years ago

#28 @kadamwhite
2 years ago

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

#29 @azaozz
2 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
2 years ago

#30 @tollmanz
2 years ago

Removing this comment as it was in the wrong ticket.

Last edited 2 years ago by tollmanz (previous) (diff)

#31 @tollmanz
2 years ago

Oopsie...wrong ticket. Sorry about that.

#32 @nacin
2 years ago

In 26136:

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

#33 @nacin
2 years ago

In 26209:

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

#34 @nacin
2 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.