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

Disable usage of var. #6639

Merged
merged 3 commits into from Jan 4, 2017
Merged

Disable usage of var. #6639

merged 3 commits into from Jan 4, 2017

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Jan 4, 2017

  • Prefer const for require's
  • Prefer let for everything else.

Adds `no-var` rule and runs `eslint --fix` across the codebase.
Strict mode is required for using `const` / `let` in module scope under Node 4.

We had previously disabled strict mode requirement for `blueprints/` directory,
due to the fact that the majority of files were actually intended for the browser
and are already receiving a global use strict.

This removes the `blueprints/.eslintrc.js` override and fixes the resulting errors
regarding missing `'use strict';` in each of the blueprint files.
const uniq = require('ember-cli-lodash-subset').uniq;
const SilentError = require('silent-error');
const sortPackageJson = require('sort-package-json');
let date = new Date();
Copy link
Contributor

Choose a reason for hiding this comment

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

i would expect this also to be const does the linting prevent this?

e.g. something that we want to share over a large context, and would prefer for the binding to not accidentally change.

Copy link
Member Author

Choose a reason for hiding this comment

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

@stefanpenner - The linting rules absolutely still allow us to use const there, but I used the basic heuristic of "if its a require then make it const" to automate these changes and this one just ended up being a let.

Once we land this we should start reviewing "whats left" and fixing things like this one...

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds great.

@stefanpenner
Copy link
Contributor

We shouldn't let this sit for long, but lets make sure we coordinate with @nathanhammond re: landing this in a way thats aligned with his plan here. (He thought about it thoroughly, don't want to mis some aspect of his plan) If that is already done, :shipit:

@rwjblue
Copy link
Member Author

rwjblue commented Jan 4, 2017

@stefanpenner - Yep, @Turbo87 @nathanhammond and I circled the wagons last night and are on the same page. We basically want to get all of this "large scale refactoring" done and out of the way before branch point into 2.12's beta cycle.

@rwjblue
Copy link
Member Author

rwjblue commented Jan 4, 2017

@homu r+

@homu
Copy link
Contributor

homu commented Jan 4, 2017

📌 Commit 19dbd27 has been approved by rwjblue

@homu
Copy link
Contributor

homu commented Jan 4, 2017

⚡ Test exempted - status

ember-cli-canary-sync pushed a commit that referenced this pull request Jan 4, 2017
Disable usage of `var`.

* Prefer `const` for `require`'s
* Prefer `let` for everything else.
@homu homu merged commit 19dbd27 into ember-cli:master Jan 4, 2017
@stefanpenner stefanpenner deleted the no-var branch January 4, 2017 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants