Add rule for const outside of module scope.

[rwjblue]

Good:

const FOO = 'FOO';

Bad:

function derp() {
  const FOO = 'FOO';
}

if (false) {
  const BLAH = 'BLAH';
}

---

I did not add configuration for this rule in the default preset (but I
can if we want it)...

[rwjblue]

Ugh, tests are failing because the rule is not enabled. I'll add a mechanism to force a given test to use a subset of rules.

[brzpegasus]

I did not add configuration for this rule in the default preset (but I can if we want it)...

I personally like the rule and would prefer that we align our best practices of const/let usage with that in Ember. We'll have an internal team discussion to make sure everyone is onboard. We can always add it to the preset later (and I think it'd be a breaking change, since the rule being enforced means existing apps that did not observe this would now see failing tests).

[brzpegasus]

I did not add configuration for this rule in the default preset (but I can if we want it)...

We decided today that we do want to start enforcing the rule as well for DockYard projects moving forward.

[cdl]

Just because I'm curious: what's the motivation/reasoning behind this (disallowing const within functions/blocks)? Typically, the other way around is what's encouraged, no? (Immutability by default, with mutability only when necessary?)

[brzpegasus]

@cdl This explanation probably sums it up the best.

[rwjblue]

Quoting @ef4 from ember-cli/ember-cli#5091 (comment):

As a rule of thumb, if it would be reasonable for someone doing a later refactor to turn your const into a let, it should have just been a let from the beginning.

Using const in a function or if is confusing. const only prevents reassignment, and does not prevent mutation (which most people assume).

const should be used only when the actual value is extremely important and cannot change. This interpretation makes it a very useful tool. The alternative is basically:

• use const initially
• decide later that you need to change the value
• replace const for let
• wut?

[poteto]

I am not a huge fan of this rule. If this is applied, would it then make sense to move variables that will never have their references mutated outside of function scope as consts?

e.g.

test('it does stuff', function(assert) {
  let expectedText = 'foo'; // never changes programatically
  assert.equal(this.$().text(), expectedText, 'should be foo');
});

vs

const EXPECTED_TEXT = 'foo';

test('it does stuff', function(assert) {
  assert.equal(this.$().text(), EXPECTED_TEXT, 'should be foo');
});

Personally, the number of times I have gone back to change a const to let is 0. I know ahead of time that I will mutate a reference, so I declare it as a let from the get go. And going back to change let expectedText = 'bar' !== mutating reference.

I am very curious to know how often going back to change const => let happens.

[brzpegasus]

I don't think we should start moving any and all variables that won't get reassigned outside the scope where they are used.

const EXPECTED_TEXT = 'foo';

test('it does stuff', function(assert) {
  assert.equal(this.$().text(), EXPECTED_TEXT, 'should be foo');
});

The expected text should stay in the test, in my opinion.

[poteto]

Then, should it just be:

test('it does stuff', function(assert) {
  assert.equal(this.$().text(), 'foo', 'should be foo');
  assert.equal(this.$('input').val(), 'foo', 'should be foo');
});

Since expectedText was an unnecessary variable?

[rwjblue]

test('it does stuff', function(assert) {
  let expectedText = 'foo'; // never changes programatically
  assert.equal(this.$().text(), expectedText, 'should be foo');
});

I don't see an issue with let here assuming that it isn't a "magic" value. If it is a magic string (comes from somewhere outside the test context/setup) then it should either be a module level const whereever it is actually "owned" (like perhaps in the component that is itself using the magic string) or encapsulated in a page object or other helper function.

[poteto]

But in that example, it is fair to assume that expectedText is absolutely not expected to mutate its reference during the test function invocation - as doing so would fail the test. Let me change the example a little to illustrate:

test('it does stuff', function(assert) {
  let frameworkName = 'Ember';
  let tagLine = 'A framework for creating ambitious web applications';
  assert.equal(this.$().text(), `${frameworkName}, ${tagLine}`, 'should be Ember');
});

or

const FRAMEWORK_NAME = 'Ember';
const TAG_LINE = 'A framework for creating ambitious web applications';

test('it does stuff', function(assert) {
  assert.equal(this.$().text(), `${FRAMEWORK_NAME}, ${TAG_LINE}`, 'should be Ember');
});

[brzpegasus]

Not necessarily. You could have this, and that should be fine:

test('it does stuff', function(assert) {
  let expectedText = 'foo';
  assert.equal(this.$().text(), expectedText, 'should be foo');

  // do more things
  // ...
  expectedText = 'bar';
  assert.equal(this.$().text(), expectedText, 'should now be bar');
});

[rwjblue]

I see nothing wrong with the example in #77 (comment), and this rule as proposed would not prevent it.

[poteto]

My main problem with this rule is that it educates developers about not abusing const (I am still uncertain why that is an issue), but instead encourages mutating things in place. Prior to this rule, the above example could have been:

test('it does stuff', function(assert) {
  const beforeText = 'foo';
  const afterText = 'bar';
  assert.equal(this.$().text(), beforeText, 'should be foo');

  // do more things
  // ...
  assert.equal(this.$().text(), afterText, 'should now be bar');
});

Which is arguably better since we know the perils of unexpected side effects. My point is that it is better to have lots of consts being used incorrectly than having it not used at all. If you don't need to reassign a variable, don't let yourself do it by mistake.

[rwjblue]

What value does this give you:

  const beforeText = 'foo';
  const afterText = 'bar';

Over:

  let beforeText = 'foo';
  let afterText = 'bar';

The test should pass or fail based on the assertions, not a random compilation error because you decided to change a local variable at some later date.

---

My main problem with this rule is that it educates developers about not abusing const (I am still uncertain why that is an issue)

It is an issue, because the cognitive overhead of choosing whether to use const (which is poorly named anyways) or let is a waste of mental energy. If your thought process is: "use const by default, and change to let if I need to mutate the reference" then what was the point of const? That thought process removes all special value that const might have, because it seems totally fine to replace const with let. In the end it served no purpose other than slowing you down when you needed to refactor.

However, things that are truly constants tend to be declared outside of function bodies which would be allowed with this rule.

[poteto]

I definitely agree that const is poor syntax. I would have preferred let and mut to replace const and let respectively, given that const is only constant by reference.

The point is to opt-in to mutation as opposed to opt-out. I think that "mental energy" cost to be extremely negligible, given that preferring const to let teaches you to avoid variable mutation, which in my opinion is a good thing. In other words, the number of times I mutate references is extremely low.

In any case, ¯_(ツ)_/¯ I can live with this rule, but I still don't think the argument for it is very compelling.

[xtian]

My thinking has been that const is a signal to future readers that the reference never changes whereas let signals the opposite. I can appreciate the value of having uniform usage though.

[brzpegasus]

I'm definitely interested to hear everyone's thoughts on the usage of const vs. let, and I appreciate the feedback. That said, I don't think they will add more value to this PR. The rule, in my opinion, has its place within ember-suave. You can always disable it if you do not want to apply it in your projects.

[rwjblue]

@brzpegasus - Thanks for summarizing, this is good to go now.

[devinus]

I am a mega 👎 for this rule as a default. I use const literally everywhere and only us e let for a binding I need to mutate exactly 3 times in my entire (huge) app.

It's refreshing to know your variable is not changing it's binding unless you've explicitly allowed it to.

[rwjblue]

@devinus: The rules are yours to customize:

{
  "preset": "ember-suave",
  "disallowConstOutsideModuleScope": false
}

The defaults have been thoughtfully considered, as you can see above.

[runspired]

@brzpegasus

routes/application.js should pass jscs.
    validateQuoteMarks: Unsupported rule: disallowConstOutsideModuleScope at routes/application.js :
         1 |import Ember from 'ember';
    --------^
         2 |
         3 |const {
         expected true

This is the result of setting it to either null or false

[brzpegasus]

FYI, the latest npm package does not have that rule yet, but I assume you're using the master branch of ember-suave?

[runspired]

oh, :P no I was using the npm release. Perils of watching repos

[brzpegasus]

No worries! I'll probably release by the end of the week.

[wycats]

Modifying height or width in this scenario ought to be an error.

I think it makes sense to use const as a note to a future editor that you do not intend to rebind the variable. I don't think it makes sense to use const purely because you happened to not rebind it (yet).

The benefit of using const this way is that it's a strong signal to future editors that switching to let should be done with care, and is probably a mistake. On the flip-side puking const everywhere will mean that people have to make pro-forma const->let changes pretty often, eliminating the value of the signal when it actually matters.

[devinus]

@wycats I think there's an even better argument for the reverse that is more concrete.

If you see let in a codebase that only uses it for it's purpose you KNOW that variable is going to be rebound somewhere within that scope. To me that is such valuable information to know immediately because of the nature of rebinding variables. Allowing the use of let elsewhere loses this massive benefit of information.

This, along with the IDE benefits of using const far outweigh what I think are subtle const usage. If there's one thing I've learned with style guides it's that trying to do subtle in a style guide never works IME.

[runspired]

@wycats @devinus I see both sides of this coin, and I'm not convinced either side is totally right. What I am convinced of is that this particular code style rule limits use of const too severely.

There is unfortunately no way to my knowledge to write a rule that simply says "you are using const here, do you mean this can never be rebound or that it currently isn't?".

In thinking more about this, I'm trending towards @wycats' opinion that the knowledge that something ought to be mut is more important than the knowledge that something is not mut. Even with that opinion though, you cannot apply that practice with this rule.

[devinus]

@runspired

I'm trending towards @wycats' opinion that the knowledge that something ought to be mut is more important than the knowledge that something is not mut

That's actually my opinion, @wycats is the reverse (i.e. the knowledge that something ought to be mut is less important than the knowledge that something is not mut, which can only be achieved by using const for non-rebinding variables, at which point it's let that has actual meaning). This rule is also simpler as it's not up to the authors finicky notion of what he thinks should be const and where let has no real meaning.

[runspired]

@devinus it actually isn't, there's a deeper level of understanding I think you missed.

Comes down to this. const could be used to signify either of two things.

• 1. a developer telling you that this variable should not change (intent + action)
• 2. a developer telling you that this variable does not change (action)

let, similarly, signifies either of two things.

• 3. a developer telling you that this variable does change (intent + action)
• 4. a developer telling you that this variable could change (action)

You opinion, as I understand it, boils down to statements (2) and (3), Wycats' boils down to (1) and (4).

e.g. The difference here is in where intent is more important. Is it more important to signify that something will be mutated? Or more important to signify that something absolutely shouldn't be mutated?

[devinus]

@runspired I definitely didn't miss it, but like I alluded to (1) and (4) are emotions and subject to differences of the authors opinion, (2) and (3) are facts about the code expressed in the code. You can tell with a quick glance at the code that a let is going to be rebound within that scope when you only use let for rebinding variables. Should not and could are not concrete. Using const for all non-rebinding variables you don't have to think, you KNOW those variables aren't being rebound anywhere.

The Erlang community loves single assignment and often touts it as a language feature that reduces stupid errors, but I often found some algorithms ugly or hard to express without the ability to rebind variables. Being able to tell by the variable declaration whether it's rebindable or not is a feature. let as a keyword gives us that, but using it elsewhere removes the advantage of guaranteed single assignment that using const gives you.

[runspired]

This is why I'm uncertain myself what the answer is, but sticking to what I've said before, regardless of the answer to the metaphysical debate here, the rule itself in question in this PR helps neither side.

[devinus]

Indeed, it doesn't represent either argument.

[devinus]

By the way, I don't think this is bikeshedding. These kinds of arguments are important when you're figuring out and imposing a style on a community. Imposing a style opinion that doesn't represent the majority of it's users is how you end up with funky shit like https://github.com/feross/standard.

[brzpegasus]

These kinds of arguments are important when you're figuring out and imposing a style on a community.

The rules defined in ember-suave represent the style we've decided to adopt at DockYard and use throughout all of our projects. Here's the first statement you'll see as you take a look at the README:

Ensures super stylish code by enforcing the DockYard JavaScript and Ember style guide rules.

We have, however, built it in such a way that it would also benefit the rest of the community, should they choose to use the addon as a starting point for style enforcement. Your opinions may align 100% with the defaults we've established, or they may align only 80%, or 90%. Either way, the preset can be customized to suit your needs by disabling those rules you would rather not see in your own projects.

[devinus]

@brzpegasus That's a good point, I didn't catch that. If this is to enforce DockYard's code style then we're indeed barking up the wrong tree.

[runspired]

I don't think it's entirely the wrong tree, this project may be Dockyard's but it's named ember-suave not dockyard-suave.

[bcardarella]

@runspired come on, the naming convention for ember addons is ember-<thing>

[runspired]

@bcardarella come on? but that's my point exactly, there's a lot to a name. If this is being promoted as an ember-addon for the community then Dockyard has less of an expectation of this being "Dockyard's exact style guide" with that name, and needs to be open to community opinions on it.

[devinus]

@runspired I honestly didn't know this was meant to reflect DockYard's style rather than a community style, but in the end it's just a name and I can blame my reading comprehension.

But maybe DockYard's Ember style guide should be updated to reflect this rule @brzpegasus ? https://github.com/dockyard/styleguides/blob/master/engineering/ember.md

[bcardarella]

@runspired so you're implying that all of your addons that are named "ember-" are the "official ember community standard" ?

The naming convention for ember addons is "ember-". This is getting absurd.

[runspired]

@bcardarella I'm implying that if ember- is there, there's an expectation that community contribution and input is possible. That's not absurd, but since I seem to have touched a nerve I won't be responding any more.

[bcardarella]

@runspired ok, I can buy that argument

[jackmatt2]

I can't believe that this got merged. You are basically telling new developers to follow bad programming practices. The const keyword is simply telling readers of you code that a variable should not be assigned to twice which is really helpful when reading other peoples code.

I really respect @rwjblue but const is not a special keyword reserved for really important variables. That is just his and others incorrect interpretation of it.

I come from a Java background and it has the final keyword which directly matches the ES6 const keyword in that it prevents reassignment of a variable but not mutability. It is common practice to use UPPER UNDERSCORE CASE for important class constants but regular camel case for constants defined inside a method.

As a contrived example (I realise that the local method variable are unnecessary here but they are in real life examples).

// As a class constants (equivalent to a "really important variable above")
public static final HELLO_WORLD = "Hello World";

public void sayHiThere() {
   // Just a regular method variable constant
   final String hiThere = "Hi There";
   System.out.println(hiThere);
}

I follow this convention in ES6 also:

// As a module constants (equivalent to a "really important variable above")
const HELLO_WORLD = 'Hello World';

export default Ember.Controller.extend({
  sayHiThere() {
    // Just a regular function variable constant
    const hiThere = 'hiThere';
    console.log(hiThere);
  }
});

This will cover all cases mentioned by @rwjblue and others.

1. A "really important variable" - Use Upper Underscore Case with type const.

   const HELLO_WORLD = 'Hello World';

2. Communicating to other developers that a variable should not be reassigned to or reused inside a function - use camel case and const keyword.

   const hiThere = 'hiThere';

What do people think of this as a convention in Ember?

[bcardarella]

@jackmatt2 the problem here is that const was a poor name chosen for what its intended purpose is. It has led to way too much confusion. For more details please see: http://madhatted.com/2016/1/25/let-it-be

[jackmatt2]

@bcardarella Most of the arguments in the article I disagree with but the one I believe does hold merit is the following:

By const being the default declaration, let rises as the more visible style of declaration. The idea is that let flags where something funny is happening. However, function arguments like function(a,b,c) { are also allowed re-assignment, so it is a false sense of security to suggest no let means no funny business is happening.

Those nasty Javascript nuances bite back!

[bcardarella]

@jackmatt2 its clear you disagree with them but unfortunately that doesn't change anything. The purpose and proper use of const has been determined by higher powers. (TC39)

[jackmatt2]

👍

[devinus]

@jackmatt2 That is easily disallowed by ESLint by the way:

http://eslint.org/docs/rules/no-param-reassign

[poteto]

I'm locking this issue. If you would like to disable this rule, simply add the following to your .jscsrc file:

{
  "preset": "ember-suave",
  "disallowConstOutsideModuleScope": false
}