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

Update: Module overrides all 'strict' rule options (fixes #4936) #4948

Merged
merged 1 commit into from Jan 19, 2016

Conversation

Projects
None yet
4 participants
@nre
Copy link
Contributor

commented Jan 14, 2016

  • strict.js: sourceType: "module" now overrides the rule option 'never'. (All other rule options were already overridden.) This changes the problem message from "Strict mode is not permitted" to "'use strict' is unnecessary inside of modules".
  • tests/strict.js: Add a test for sourceType: "module" with the rule option 'never'. (There already were tests for sourceType: "module" with the 'global' and 'function' options.)
  • rules/README.md and strict.md:
    • Clarify that the 'strict' rule concerns strict-mode directives, not strict mode itself.
    • Mention that ECMAScript modules are always in strict mode.
    • Document that sourceType: "module" overrides the rule options.
    • Consistent use of strict-mode directive instead of the previous mix of names:
      • Use Strict Directive
      • "use strict" directive
      • strict directive
    • Consistent use of hyphens in the prose.

@eslintbot eslintbot added the cla found label Jan 14, 2016

@@ -1,12 +1,12 @@
# Strict Mode (strict)
# Strict-Mode Directives (strict)

This comment has been minimized.

Copy link
@nzakas

nzakas Jan 14, 2016

Member

Let's keep using "strict mode" (no dash) throughout, as I don't usually see it hyphenated.

//--------------------------------------------------------------------------

if (mode === "never") {
if (context.parserOptions.sourceType === "module") {

This comment has been minimized.

Copy link
@nzakas

nzakas Jan 14, 2016

Member

You should check the program node for sourceType, as not all parsers will support parsing as a module using parser options.

This comment has been minimized.

Copy link
@nre

nre Jan 14, 2016

Author Contributor

Five rules use sourceType:

  • no-eval
  • no-invalid-this
  • no-redeclare
  • no-use-before-define
  • strict

All use context.parserOptions.sourceType; none uses sourceType on the Program node. This is probably because 'migrating-to-2.0.0.md' says:

If you're using context.ecmaFeatures.modules, rewrite to check for context.parserOptions.sourceType === "module"`.

This comment has been minimized.

Copy link
@nzakas

nzakas Jan 14, 2016

Member

Hmm, wasn't aware of that. It's true that context.ecmaFeatures.modules is the same as context.parserOptions.sourceType in that both are just flags to the parser, but those are probably not the best way to check if the parsed code represents a module. I'll update the migration docs. For now, can you make the change in this rule? We can worry about the others later.

This comment has been minimized.

Copy link
@mysticatea

mysticatea Jan 15, 2016

Member

This rule is selecting a returning object by the option, so I think it introduces complexity to this rule if it uses sourceType of Program node.

@nre nre force-pushed the nre:issue4936 branch from bf3065a to 877fbfc Jan 15, 2016

@nre

This comment has been minimized.

Copy link
Contributor Author

commented Jan 15, 2016

Requested changes made:

  • rules/strict.js: Get sourceType from Program node, instead of from context.parserOptions.
  • rules/README.md and strict.md: Remove hyphens between 'strict' and 'mode'.

@nre nre force-pushed the nre:issue4936 branch from 877fbfc to 05b8cb3 Jan 19, 2016

@nre

This comment has been minimized.

Copy link
Contributor Author

commented Jan 19, 2016

Resolved a merge conflict with #4995.

@nzakas

This comment has been minimized.

Copy link
Member

commented Jan 19, 2016

Lgtm

nzakas added a commit that referenced this pull request Jan 19, 2016

Merge pull request #4948 from nre/issue4936
Update: Module overrides all 'strict' rule options (fixes #4936)

@nzakas nzakas merged commit 0acbe57 into eslint:master Jan 19, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@eslint eslint bot locked and limited conversation to collaborators Feb 6, 2018

@eslint eslint bot added the archived due to age label Feb 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.