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

`strict` option should take `env` into account #3306

Closed
sindresorhus opened this Issue Aug 7, 2015 · 13 comments

Comments

Projects
None yet
7 participants
@sindresorhus
Copy link
Contributor

commented Aug 7, 2015

http://eslint.org/docs/rules/strict

Problem

In Node.js it makes sense to use the global mode as files are implicitly wrapped in an IIFE by the runtime so you can safely put the use strict statement at the top of the file, but in the browser you'd want to use the function mode as you don't want to leak the use strict statement.

Solution

Implement an auto mode to the strict option, that uses global if env is node, otherwise function.

@eslintbot

This comment has been minimized.

Copy link

commented Aug 7, 2015

Thanks for the issue! We get a lot of issues, so this message is automatically posted to each one to help you check that you've included all of the information we need to help you.

Reporting a bug? Please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. The source code that caused the problem
  3. The configuration you're using (for the rule or your entire config file)
  4. The actual ESLint output complete with line numbers

Requesting a new rule? Please be sure to include:

  1. The use case for the rule - what is it trying to prevent or flag?
  2. Whether the rule is trying to prevent an error or is purely stylistic
  3. Why you believe this rule is generic enough to be included

Requesting a feature? Please be sure to include:

  1. The problem you want to solve (don't mention the solution)
  2. Your take on the correct solution to problem

Including this information in your issue helps us to triage it and get you a response as quickly as possible.

Thanks!

sindresorhus added a commit to xojs/eslint-config-xo that referenced this issue Aug 7, 2015

disable `strict` rule for now
It didn't support the browser use-case. See: eslint/eslint#3306
@nzakas

This comment has been minimized.

Copy link
Member

commented Aug 7, 2015

Rules cannot change their behavior based on which environments are used. Keep in mind, you can apply both node and browser environments at the same time. Can you explain more about what you're trying to do?

@gyandeeps gyandeeps added the triage label Aug 7, 2015

@silverwind

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2015

The leaked strict mode is a concern when you bundle up scripts written for different modes. I'm not sure if a 'use strict' gets hoisted or not, but it sure will affect all scripts following it.

Unfortunately, it's probably impossible to determine whether a file is going to be ran in a browser or node.

@BYK

This comment has been minimized.

Copy link
Member

commented Aug 7, 2015

I also have this problem and had to disable this rule when transitioning into v1.0.0

The problem is Gruntfile being run on NodeJS but most other files running in the browser via AMD. We want the "function" version for browser files and the global one for the ones running on NodeJS.

@sindresorhus

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2015

@nzakas I want to be able to use a sharable config with the strict option and have the strict option adjust acordingly when I toggle an env on a file by file basis.

gulpfile (Node.js)

/*eslint-env node */

...

main.js (browser)

/*eslint-env browser */

...
@silverwind

This comment has been minimized.

Copy link
Contributor

commented Aug 25, 2015

I think one way of solving this is to add an option like

strict: [2, "env", {node: "global", browser: "function"}]

which then checks for an eslint-env comment in the file. If no comment is found, there should be an error on line 0 to set one.

@nzakas

This comment has been minimized.

Copy link
Member

commented Aug 25, 2015

As I mentioned, rules don't know anything about environments. That's by design, since rules shouldn't be keying off of environments, but rather key off of what it actually finds in the code or scope.

We might be able to do something using the globalReturn language feature, though. That's true for the node environment, so it's a soft indicator of the same thing. The question would still be how to provide an option that makes sense.

@btmills

This comment has been minimized.

Copy link
Member

commented Aug 25, 2015

What about smart? Maps to global when globalReturn is true, otherwise function?

@sindresorhus

This comment has been minimized.

Copy link
Contributor Author

commented Aug 25, 2015

which then checks for an eslint-env comment in the file. If no comment is found, there should be an error on line 0 to set one.

The point isn't to enforce the eslint-env comment, but rather look at the resolved config for a file. If env contains node but not browser, enforce the "use strict" globally. If env contains browser but not node, enforce function scoped "use strict". If env contains both, do nothing.

@nzakas

This comment has been minimized.

Copy link
Member

commented Aug 26, 2015

@btmills I kind of hate the term "smart" (one of the most overused for technical things: smart search, smart tabs, etc.), but I think you're thinking in the right direction. I'm also not sure I can come up with anything better, so I'm willing to go with it.

@btmills

This comment has been minimized.

Copy link
Member

commented Aug 28, 2015

I agree with you there. Brainstorming a couple others: auto, detect, ...

@nzakas

This comment has been minimized.

Copy link
Member

commented Aug 28, 2015

"safe"? As in, is safe to have " use strict" at the top level for Node but not for other places?

@btmills

This comment has been minimized.

Copy link
Member

commented Aug 28, 2015

👍 for "safe"

@nzakas nzakas added enhancement rule accepted and removed triage labels Aug 31, 2015

btmills added a commit that referenced this issue Dec 12, 2015

@btmills btmills closed this in 17c7624 Dec 12, 2015

nzakas added a commit that referenced this issue Dec 12, 2015

Merge pull request #4679 from eslint/issue3306
Update: Add "safe" mode to strict (fixes #3306)

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

@eslint eslint bot added the archived due to age label Feb 7, 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.