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

Different settings for different JS versions #159

Closed
julien-f opened this issue Jun 15, 2015 · 51 comments
Closed

Different settings for different JS versions #159

julien-f opened this issue Jun 15, 2015 · 51 comments

Comments

@julien-f
Copy link

julien-f commented Jun 15, 2015

I think it would make sense for some settings to differ based on which JS version is targeted:

Of course it would be the best if compatibility is kept (a JS3 code is compatible with JS3 up to JS6 config).

I don't know what would be the best way to implement that, maybe via an options in package.json:

{
  "standard": {
    "compatibility": "es2015"
  }
}
@julien-f
Copy link
Author

I think prefer-const can be used unconditionally because it only applies to lets, not vars.

@julien-f
Copy link
Author

No opinions on this?

@feross
Copy link
Member

feross commented Jun 24, 2015

I can see wanting to use no-var and object-shorthand on ES6 codebases -- those seem useful.

Personally, I don't think always using const when possible actually helps to catch real-world bugs. If I used it, I'd only want to do it for actual ALL_CAPITAL constants that I declare at the top of my file. Doing it for every single variable that happens to not be modified seems like overkill, and won't actually catch bugs, just create extra work.

Not a fan of comma dangle. Currently, we actively disallow it. Enabling it would cause almost every single dependent to start failing -- can't do that.

The question is how do get no-var and object-shorthand for people on ES6 codebases. Are these things always going to require options?

@LinusU
Copy link
Member

LinusU commented Jun 24, 2015

I think this is a very good idea.

Would it also make sense to enable specific features? I currently compile my javascript as below and would love to have console.log.bind(console) to be disallowed in favour of ::console.log.

babel src -s --optional es7.functionBind --out-dir lib

no-var is really good because I always miss it in at least a few places in the file.

I'm on the fence about comma dangle but to be fair it does make it easier to move lines up and down. Often when I do that there will be a line in the middle without comma which makes for a syntax error.

@feross What do you mean with this?

Are these things always going to require options?

As long as we specify somehow that the code is ECMAScript 6 it shouldn't require any more options. There is no reason to use var in ES6 code.

@julien-f
Copy link
Author

I disagree: declaring vars with const instead of let does not require extra work but forces the dev to explicitly indicate when he intends to modify a variable. I think it encourages immutability.

I agree forcing comma dangle is not viable solution as it would break compatibility but allowing it for ES>3 would not create any issues and allow cleaner diffs.

@LinusU
Copy link
Member

LinusU commented Jun 24, 2015

I really think that we should either enforce it or forbid it. Letting the user decide for themselves just creates more bike-sheding?

@feross
Copy link
Member

feross commented Jun 24, 2015

Remember, you can always "fork" standard if you care a ton about a single rule. Add your own rule with .eslintrc like this:

{
  "extends": ["standard", "standard-react"],
  "rules": {
    "comma-dangle": [2, "always"]
  }
}

@yoshuawuyts
Copy link
Contributor

Defaulting to const seems like a good thing; if all code is moving to be block-scoped anyway const seems like the better solution. I've been using it for a while, and it's helped me catch at least a few bugs in longer files that hadn't been written with care.

I also agree with @julien-f that using const / let specifies intent more clearly; having mutation be the exception rather than the rule makes code easier to reason about for reviewers and authors alike.

@dcousens
Copy link
Member

I can see wanting to use no-var and object-shorthand on ES6 codebases -- those seem useful.

Agreed.

Personally, I don't think always using const when possible actually helps to catch real-world bugs. If I used it, I'd only want to do it for actual ALL_CAPITAL constants that I declare at the top of my file. Doing it for every single variable that happens to not be modified seems like overkill, and won't actually catch bugs, just create extra work.

Although I am usually a massive proponent for const, I think you may be right.
const gives us a tonne of benefits in statically typed languages because it can provide guarantees about the data itself, allowing us to strongly enforce functional purity.

This is not so in JS, as the constness stops at the top level assignment, and is not transitive in any way.

So, while it pains me to say it, I think prefer-const should remain disabled.

've been using it for a while, and it's helped me catch at least a few bugs in longer files that hadn't been written with care.

@yoshuawuyts maybe I'm just being short sighted, but can you elaborate on this?

I think it encourages immutability.

@julien-f this is the one positive I can see. Immutability rules. But IMHO, it is almost always better to be enforced through libraries like immutable.js then it is at the scope level.
Perhaps this rule could help push in that direction though?

The question is how do get no-var and object-shorthand for people on ES6 codebases. Are these things always going to require options?

We should ask eslint if we we can add an option OPTION-if-es6 (like, always-if-es6) or something, maybe a way to override rules if es6 is detected in anyway.

That way we can solve a multitude of issues simultaneously!

(e.g #167)

@yoshuawuyts
Copy link
Contributor

@dcousens I was leading a refactoring effort at a previous job where ~1k-2k LOC files weren't uncommon. At that point it was impossible to remember all variables available in scope, and accidentally redeclaring variables happened. const provided a mechanism to warn about this, what else would've been manual debugging. Arguably eslint:no-redeclare serves the same purpose, so maybe that point is moot for projects that rely on standard / eslint. (ツ)_/¯

Actually I'm curious what the reasons against prefer-const are / why let is preferred. Off the top of my head:

pro const

  • intent is communicated explicitely
  • once past the learning curve it's not more work (based on my own experience)
  • possibly faster (vm optimizations?)
  • discourages random mutations (to some extent)
  • works in node>=0.10 where let doesn't

con(st) const

  • breaks existing modules (not a problem if an es6 flag can be enabled)
  • using const + let requires different reasoning about variables (learning curve)
  • two characters longer than let
  • const allows mutating object properties, which can be confusing for newcomers

Does that sound about right? Let me know if I missed anything.

@dcousens
Copy link
Member

Looks right to me.
I guess a pro could be encouraging immutability as well.

@feross
Copy link
Member

feross commented Jun 25, 2015

Here's a concrete problem I have with prefer-const. Look at this code:

'use strict'
const http = require('http')

function fetchGoogle (cb) {
  let options = {
    hostname: 'www.google.com',
    port: 80,
    path: '/'
  }

  if (Math.random() > 0.5) options.headers = { DNT: 1 }

  http.get(options, cb)
}

fetchGoogle(function (res) {
  console.log('got ' + res.statusCode)
})

With prefer-const, I get this error:

t.js:6:6: `options` is never modified, use `const` instead.

My problems with const in this situation are:

  1. The properties on options are actually modified, so const isn't providing any real protection here, and it's just plain confusing.
  2. Having to const every little helper variable, like options in this example, will generate a ton of errors, and I'm not convinced it's worth the cost.

@julien-f
Copy link
Author

@feross It's because the semantic of const is not obvious, it guarantee that the variable will always point to the same value, not that this value will not be changed.

If, in Node, exports was declared as const exports = modules.exports it would have prevented tons of mistakes from beginners which affects an object to exports instead of module.exports.

@feross
Copy link
Member

feross commented Jun 25, 2015

@julien-f I think that's a pretty weak guarantee and it's not worth forcing users to use it in every possible situation where it could conceivably be used. Rules have to be worth the pain they cause, and I'm not yet convinced this rule is worth it.

@julien-f
Copy link
Author

I can understand your arguments against the semantic of const but I really don't see what the pain is in using const instead of let, it costs absolutely no effort.

@LinusU
Copy link
Member

LinusU commented Jun 25, 2015

This is very subjective and personal but I don't really like the look of const. It feels weird to mix 3 chars and 5 chars when defining my list of variables.

function complicatedStuff () {
  let i = 0
  const len = a.length
  let futureKey = 'next'
  let lastKey
  const firstKey = futureKey

  // ...
}

But what do you guys think about enforcing const to be used on ALL_CAPS variables? I think that that would be a great first step!

const ENCODING = 'utf-8'

const OP_WRITE = 0x00
const OP_READ = 0x10

@julien-f
Copy link
Author

@LinusU Since let and const are block scoped, I rarely have a list of variables declarations anymore.

@dcousens
Copy link
Member

The properties on options are actually modified, so const isn't providing any real protection here, and it's just plain confusing.

Yup, non-transitive const is pants on head retarded IMHO.

But what do you guys think about enforcing const to be used on ALL_CAPS variables? I think that that would be a great first step!

I think that seems fair.

@feross
Copy link
Member

feross commented Jul 1, 2015

Btw, in the last version of standard, I enabled the following ES6 rules:

These rules are nice because they don't affect non-ES6 code. We can continue to enable these types of rules with no issues :)

@hax
Copy link

hax commented Nov 13, 2015

@feross
What const in JS have the similar semantic with const in C++, or final in Java, or readonly in C# (but C# only allow readonly for field, not local var), but not const in PHP/Go/C#...

It's not weak guarantee, it just only guarantee the value (or reference in object case) can not be modified.

In C++, you should also use two const in declaration (eg. int const * const) to guarantee both aspect.

In JS, we could use Object.seal() to guarantee the immutability of the object now, though no one use it (partly because engines do bad optimization).
The better choice is struct (syntax like const immutableOptions = #{a: ..., b: ...} as the draft).

Notice it's far more ugly in Java than JS (in Java final int vs int is much longer and distract than const vs let in JS), but many people in Java community still prefer final for local var (Eclipse and Netbean support auto prefix it for all local var), for example, Andorid project use such coding style.

To be honest, const is a bad name, JS should use let for current const semantic, and use let var for current let semantic. (Rust use let vs let mut, Swift use let vs var, Scala use val vs var.)

Anyway, it's impossible to change the bad names now, what we can do is educating the js programmers to familiar the usage of const/let. So I suggest enable prefer-const by default.

@julien-f
Copy link
Author

@hax Thank you very much for this extremely clear argument :)

@hax
Copy link

hax commented Nov 13, 2015

And FYI, airbnb already enable prefer-const, see: https://github.com/airbnb/javascript/blob/master/packages/eslint-config-airbnb/rules/es6.js#L43

@dcousens
Copy link
Member

If we ever switch to ES6 only, reference standard/eslint-config-standard-react#7 might be worth including.

@justmoon
Copy link

One more thought that I didn't see mentioned yet: let vs const communicates developer intent. I may want to define a variable with let to indicate that you should expect that the value of this variable could change, even if it currently doesn't.

@haadcode
Copy link

I would also like to see comma-dangle and prefer-const as proposed here in the standard.

@yoshuawuyts
Copy link
Contributor

I reckon opinions on dangling commas are divided, and thus that the best resolution in such a case is to not break backwards compatibility. I propose we don't break codebases.

In that spirit I also dislike the prefer-const rule as forcing ES6 onto people is a bad thing. And yeah let's also not add options to enable different modes of standard because not having options is kind of the point of standard hah.

@yoshuawuyts
Copy link
Contributor

Oh I also don't like option b: loosening rules as it makes a codebase more inconsistent which like also goes against the point of having a linter. I think we shouldn't force everyone to suddenly use dangling commas

@julien-f
Copy link
Author

I reckon opinions on dangling commas are divided, and thus that the best resolution in such a case is to not break backwards compatibility. I propose we don't break codebases.

It does not break any code and people can keep using standard 8 if they don't want to use this feature.
It's the main purpose of standard to choose the “best” coding style after weighting options.

I also dislike the prefer-const rule as forcing ES6 onto people is a bad thing.

prefer-const does not force ES6, it only applies to let, not var.

@haadcode
Copy link

haadcode commented Nov 21, 2016

I reckon opinions on dangling commas are divided, and thus that the best resolution in such a case is to not break backwards compatibility. I propose we don't break codebases.

It's the main purpose of standard to choose the “best” coding style after weighting options.

I would also propose to look at the reasons behind the decisions and not judge purely on opinions or consensus of opinions.

For comma-dangle there are very good reasons to enable it as described on the blog post @diasdavid linked above. For me the biggest reason to enable it would be the diffs, not so much code writing part. I hate to decode whenever there's a 2 line diff related to dangling comma to understand what exactly was added and what was removed. Enabling dangling commas would allow "true" single-line changes, much like in Go https://dave.cheney.net/2014/10/04/that-trailing-comma.

@omeid
Copy link

omeid commented Jan 7, 2017

At the cost of beating the dead horse, I would also like to see comma-dangle enabled, the background compatiblity issue is not a big deal with --fix as @julien-f mentioned.

I am not sure what woudl be the implications of making comma-dangle optional?

@stale
Copy link

stale bot commented May 10, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale label May 10, 2018
@julien-f
Copy link
Author

Hey, I believe this issue is still relevant 🙂

@stale stale bot removed the stale label May 10, 2018
@kyeotic
Copy link

kyeotic commented Jun 12, 2018

I would like to side against prefer const. Just because I am not mutating a variable at this point in time does not mean I intend the variable to be immutable in the future. I may intend for the variable to be changeable, and I indicate that by declaring it as let. The prefer-const rule stops me from signaling that future intent.

@julien-f
Copy link
Author

@tyrsius It's probably pretty rare, maybe it would be preferable to simply opt-out (// eslint-disable-line prefer-const) the few times you need this behavior 🙂

@kyeotic
Copy link

kyeotic commented Jun 12, 2018

@julien-f I already have a way to opt-out of that behavior: use let. let and const are signals, and I like having them both.

@julien-f
Copy link
Author

Let's agree to disagree of the meaning of these signals: for me let means that the value of this variable will change during its lifetime, and const that it will not.

@kyeotic
Copy link

kyeotic commented Jun 12, 2018

I agree with you on the meaning of those signals. Our disagreement seems to be that I want to consider potential futures, where as you only want to consider the current state of the code.

@sara0871
Copy link

sara0871 commented Jul 4, 2018

👍

@stale
Copy link

stale bot commented Oct 2, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale label Oct 2, 2018
@stale stale bot closed this as completed Oct 9, 2018
@julien-f
Copy link
Author

julien-f commented Oct 9, 2018

FYI, I've started using more and more ESLint instead of Standard exactly for this reason (being able to target a specific version of ES, like ES5).

@LinusU
Copy link
Member

LinusU commented Oct 10, 2018

@julien-f which rules is it that you enable on top of standard? Maybe it's time to just include them 🤔

@julien-f
Copy link
Author

I've listed the main one in the first message (comma-dangle is a must for code reviews!), but also the ability to set the ecmaVersion to ensure the code is strictly ES3/5/2015 etc.

@LinusU
Copy link
Member

LinusU commented Oct 10, 2018

Ah, right, sorry didn't look at the top comment 😄

Oh, how I would love to have comma-dangle 😄 I don't know how many times I've talked about starting trailing-standard with various people 😂 Unfortunately it would break a lot of the ecosystem...

I don't see a problem with adding prefer-const right away since it only complains when using let instead of const, and where let is supported, const is as well.

Personally, I would like to see no-var and object-shorthand just enabled in the next major. If anyone is targeting an older environment, they can simply stay on 12.x. We could maintain that line as well for some time as well...

@LinusU
Copy link
Member

LinusU commented Oct 10, 2018

@julien-f put up a PR for prefer-const ☺️ standard/eslint-config-standard#133

@LinusU
Copy link
Member

LinusU commented Oct 10, 2018

Hmmm, no-var is not looking too good 😅

# tests 265
# pass  47
# fail  218

object-shorthand:

# tests 265
# pass  171
# fail  94

Hmm, I would like to do some more research and see how many of those are actually targeting older environments 🤔 💭

@lock lock bot locked as resolved and limited conversation to collaborators Jan 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Archived in project
Development

No branches or pull requests