Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[[FEAT]] Implement new option futurehostile #2172

Merged
merged 1 commit into from Feb 23, 2015

Conversation

jugglinmike
Copy link
Member

A recent change to JSHint included the ECMAScript 6 global identifiers
in JSHint's collection of "predefined" variables. This disallowed the
definition of those global values even in contexts that did not
implement them.

The intent of this change was to help users avoid bugs when migrating to
the new version of the language. Re-using an existing warning mechanism
caused confusion among many users [1][2][3] for two reasons:

  1. The generated warning messages were somewhat misleading (i.e. the
    warning "Redefinition of 'Promise'." is inaccurate in ES5
    environments)
  2. The method of disabling the warnings did not accurately communicate
    the intent of the developer (i.e. setting predef: -Promise does not
    prompt the user to consider the user to consider if creating their
    own global variable named Promise is appropriate)

A dedicated warning message more clearly describes the best practice
that motivates this particular constraint on global variable names. An
explicit option makes it less likely that users will disable this
warning without understanding the motivation.

Intoduce a new option, futurehostile, and dedicated warning message to
better communicate the intent of restricting usage of future-reserved
identifiers.

[1] GH-1747 redefinition of Promise (W079)
[2] GH-1995 ES6 globals are activated even though esnext=false
[3] GH-2171 Remove Set from the ECMAScript5 reserved words

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 94.86% when pulling 9b06eaf on jugglinmike:futurehostile into 4e59553 on jshint:master.

Reflect : false,
Symbol : false,
System : false
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely like this

nit: alphabetize lists like this as we go along—thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should add:

5: {
  JSON: false
}

I don't think we need to go pre-ES3

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does the System object still exist? It was removed from the draft, and I thought it was going to be defined in a separate document but I haven't seen it anywhere if that happened

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, it was removed—I wonder if JSHint should claim domain over the separate spec as well?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 94.86% when pulling c750f3f on jugglinmike:futurehostile into 4e59553 on jshint:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 94.86% when pulling c750f3f on jugglinmike:futurehostile into 4e59553 on jshint:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 94.86% when pulling 0f1c8e0 on jugglinmike:futurehostile into 4e59553 on jshint:master.

@jugglinmike
Copy link
Member Author

@rwaldron @caitp Thanks for the review! A couple things:

  • Removing System is a breaking change. I think this is fair because developers should generally recognize that ES6 is a moving target and that enabling esnext will tend to decrease stability
  • The most recent commit repurposes the existing ecmaIdentifiers attribute. Nobody asked for this, but after moving JSON to ES5, it only seemed appropriate to put everything else in ES3 so we'd have a consistent interface for referencing all of these globals.

},
5: {
JSON : false
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to really correct, split up 3 into 1, 2 & 3... But that might be a bit much

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do want to be really correct, but I agree that it would be overkill since (currently) there is no way to tell JSHint that you are operating in a pre-ES3 environment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this (numbered versions) convention going to confuse people if the year-based (eg ES2016™) scheme becomes widely known/popular?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nevermind, I guess the numbers aren't really exposed via options or anything yet

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this (numbered versions) convention going to confuse people if the year-based (eg ES2016™) scheme becomes widely known/popular?

The spec will still have numbered editions, only the title will change. For example: "ECMAScript 2015 Language Specification" is the same document as "ECMA-262, 6th Edition"

@rwaldron
Copy link
Member

@jugglinmike this needs a rebase

@jugglinmike
Copy link
Member Author

@rwaldron Sure thing; should I squash while I'm at it?

A recent change to JSHint included the ECMAScript 6 global identifiers
in JSHint's collection of "predefined" variables. This disallowed the
definition of those global values even in contexts that did not
implement them.

The intent of this change was to help users avoid bugs when migrating to
the new version of the language. Re-using an existing warning mechanism
caused confusion among many users [1][2][3] for two reasons:

1. The generated warning messages were somewhat misleading (i.e. the
   warning "Redefinition of 'Promise'." is inaccurate in ES5
   environments)
2. The method of disabling the warnings did not accurately communicate
   the intent of the developer (i.e. setting `predef: -Promise` does not
   prompt the user to consider the user to consider if creating their
   own global variable named `Promise` is appropriate)

A dedicated warning message more clearly describes the best practice
that motivates this particular constraint on global variable names. An
explicit option makes it less likely that users will disable this
warning without understanding the motivation.

Intoduce a new option, `futurehostile`, and dedicated warning message to
better communicate the intent of restricting usage of future-reserved
identifiers.

[1] jshintGH-1747 redefinition of Promise (W079)
[2] jshintGH-1995 ES6 globals are activated even though esnext=false
[3] jshintGH-2171 Remove Set from the ECMAScript5 reserved words
@rwaldron
Copy link
Member

Sure, that will be nice and clean

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 95.53% when pulling da52aa0 on jugglinmike:futurehostile into 69b3187 on jshint:master.

@jugglinmike
Copy link
Member Author

@rwaldron All set

rwaldron added a commit that referenced this pull request Feb 23, 2015
[[FEAT]] Implement new option `futurehostile`
@rwaldron rwaldron merged commit bfcc86d into jshint:master Feb 23, 2015
jugglinmike pushed a commit to jugglinmike/jshint that referenced this pull request Feb 28, 2015
[[FEAT]] Implement new option `futurehostile`
jugglinmike pushed a commit to jugglinmike/jshint that referenced this pull request Feb 28, 2015
[[FEAT]] Implement new option `futurehostile`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants