Make WordPress Core

Opened 10 years ago

Closed 5 months ago

#28236 closed task (blessed) (fixed)

JSHint `onevar` and `trailing` args are deprecated and dropped

Reported by: georgestephanis's profile georgestephanis Owned by: swissspidy's profile swissspidy
Milestone: 6.5 Priority: normal
Severity: normal Version: 3.8
Component: Build/Test Tools Keywords: has-patch commit
Focuses: javascript Cc:

Description

As per http://www.jshint.com/docs/options/#onevar

These options are deprecated and will be removed soon. DO NOT use them.

This was brought up by rase- on GitHub to Jetpack here: https://github.com/Automattic/jetpack/pull/432#issuecomment-42783894

Unsure of the best route forward.

Attachments (2)

28236.diff (310 bytes) - added by netweb 9 years ago.
28236.2.diff (861 bytes) - added by swissspidy 5 months ago.

Download all attachments as: .zip

Change History (21)

#2 follow-up: @scott.gonzalez
10 years ago

JSHint is removing options that are purely style related. The use of onevar doesn't affect actual code quality or prevent bugs, so it's being removed. There are a few options for dealing with this.

You can move to another tool for style checks, such as JSCS or esformatter. Both are being constantly improved and have a focus on handling the style related options being removed form JSHint. jQuery team members are currently investing in both solutions (we now have at least one maintainer on each project).

Another option is to rebuild the style options as extensions for JSHint. JSHint's API is being redesigned to make extensions/plugins easy to add, so this will likely happen by someone in the community anyway.

A final option is to just do nothing. As long as JSHint 2.x is working well for you, there's no real need to upgrade. Waiting to see if the community settles on a single solution seems like a legitimate option.

#3 @jorbin
10 years ago

  • Milestone changed from Awaiting Review to Future Release

I think the best solution is going to be to wait and see how this settles out. I'm more familiar with JSCS, but the potential for this to be a jshint plugin might make the most sense. JSHint is working fine for us now and we don't need to upgrade it just yet.

When we need to upgrade JShint to a new version, we can make a decision then. Until then, I don't think we need to rush to make one.

#4 @kadamwhite
10 years ago

Agreed with "wait and see." I too have used (and like) JSCS, and JSCS or equivalent would also let us specify some of the other stylistic rules (whitespace around function arguments, etc) that currently exist by convention only; however, jumping into that water may be premature right now. We'd be better served to focus on enforcing curly or eqeqeq than worrying about upgrading our toolset.

#5 in reply to: ↑ 2 ; follow-up: @azaozz
10 years ago

Replying to scott.gonzalez:

The use of onevar doesn't affect actual code quality or prevent bugs...

That's not exactly true. Example:

var a = 1;
function test() {
  console.log(a);
  var a = 2;
}
test();

Can you guess what a would be before it is defined in local scope? :)

#6 in reply to: ↑ 5 ; follow-up: @scott.gonzalez
10 years ago

Replying to azaozz:

Replying to scott.gonzalez:

The use of onevar doesn't affect actual code quality or prevent bugs...

That's not exactly true. Example:

That's not related to onevar since you have two different functions, each one containing a single var statement.

#7 in reply to: ↑ 6 ; follow-up: @azaozz
10 years ago

Replying to scott.gonzalez:

OK, expanded example:

a = 1;
function test() {
  var b = 2;
  // more code
  
  if ( a === 1 ) {
    // can't do that, a is unreachable
  }

  var c = 3;
  // more code

  var a = 4;
  // more code
}

It's somewhat comparable to the "Yoda" conditionals, etc. Agree this is very rare, but not impossible.

#8 in reply to: ↑ 7 @scott.gonzalez
10 years ago

Replying to azaozz:

Replying to scott.gonzalez:

OK, expanded example:

JSHint will actually complain that a is already defined in that scenario. Anyway, if your intention is to show that this block doesn't do what you expect because you're overwriting a, using onevar won't stop that logical mistake.

If you want to prove that onevar isn't a style-related option, you should test it against JSHint and make your case to the maintainers of JSHint. Arguing it here isn't going to help the situation in any way.

Last edited 10 years ago by scott.gonzalez (previous) (diff)

#9 @johnbillion
10 years ago

  • Version changed from trunk to 3.8

#10 follow-up: @netweb
9 years ago

  • Summary changed from JSHint `onevar` arg is being deprecated and dropped to JSHint `onevar` and `trailing` args are deprecated and dropped

A few things here....

Since we are now using JSHint v2.5 via grunt-contrib-jshint v0.10.0 in our package.json the onevar and trailing options in .jshintrc are pointless, support for both of these was removed in JSHint v2.5.

I've had my head in JSCS for the past couple of days and I'll create a JSCS ticket and link back here once done to investigate JSCS options further and to add support for these now removed onevar and trailing options via JSCS.

JSCS includes equivalent options for ease of migrating settings from JSHint to JSCS

@netweb
9 years ago

#11 @netweb
9 years ago

  • Keywords has-patch added

#12 @netweb
9 years ago

Related: #31823 Add ESLint integration

Last edited 8 years ago by netweb (previous) (diff)

#13 follow-up: @afercia
9 years ago

just ran grunt jshint:core --file=updates.js and didn't get a double var, "onevar" option was removed in https://github.com/jshint/jshint/releases/tag/2.5.0 but still set on WP .jshintrc
Just as update :)

Last edited 9 years ago by afercia (previous) (diff)

#14 in reply to: ↑ 13 @netweb
9 years ago

Replying to afercia:

just ran grunt jshint:core --file=updates.js and didn't get a double var, "onevar" option was removed in https://github.com/jshint/jshint/releases/tag/2.5.0 but still set on WP .jshintrc
Just as update :)

Thanks, and you are correct, JSHint doesn't actually throw a warning notice for deprecated options.
https://github.com/jshint/jshint/blob/ee436b160ad5ce146f730b7f8ea394ff4bb864a1/src/options.js#L939

#15 in reply to: ↑ 10 @netweb
8 years ago

Replying to netweb:

JSCS includes equivalent options for ease of migrating settings from JSHint to JSCS

The equivalent ESLint rules:

Edit: Both the above ESLint rules will be included in eslint-config-wordpress v1.1.0

Last edited 8 years ago by netweb (previous) (diff)

#16 @netweb
7 years ago

#43135 was marked as a duplicate.

@swissspidy
5 months ago

#17 @swissspidy
5 months ago

  • Keywords commit added; dev-feedback removed
  • Milestone changed from Future Release to 6.5
  • Type changed from defect (bug) to task (blessed)

This just came up again while reviewing https://github.com/WordPress/wordpress-develop/pull/6165 for #54370

I really want to tackle ESLint soon (#31823), but in the meantime, fixing this outdated config makes sense. It's silly to prevent things like trailing commas in new code. Also, we now have Prettier for code formatting as well.

#18 @swissspidy
5 months ago

  • Owner set to swissspidy
  • Status changed from new to assigned

#19 @swissspidy
5 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 57702:

Build/Test Tools: Update JSHint config to remove deprecated options.

Removes deprecated options that no longer have any effect, and updates the targeted ES version in line with WordPress’ browser support.

This change mostly allows new code to properly use trailing commas, as added by the Prettier formatter.

Future efforts should rather go towards adopting ESLint for code formatting, see #31823.

Props netweb.
Fixes #28236.

Note: See TracTickets for help on using tickets.