WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 14 months ago

#28236 new defect (bug)

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

Reported by: georgestephanis Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.8
Component: Build/Test Tools Keywords: dev-feedback has-patch
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 (1)

28236.diff (310 bytes) - added by netweb 3 years ago.

Download all attachments as: .zip

Change History (16)

#2 follow-up: @scott.gonzalez
4 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
4 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
4 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
4 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
4 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
4 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
4 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 4 years ago by scott.gonzalez (previous) (diff)

#9 @johnbillion
3 years ago

  • Version changed from trunk to 3.8

#10 follow-up: @netweb
3 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
3 years ago

#11 @netweb
3 years ago

  • Keywords has-patch added

#12 @netweb
3 years ago

Related: #31823 Add ESLint integration

Last edited 20 months ago by netweb (previous) (diff)

#13 follow-up: @afercia
3 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 3 years ago by afercia (previous) (diff)

#14 in reply to: ↑ 13 @netweb
3 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
20 months 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 14 months ago by netweb (previous) (diff)
Note: See TracTickets for help on using tickets.