From 1c509581da0af6b341166bf1ed76c5c5667e0fac Mon Sep 17 00:00:00 2001 From: Sang Mercado Date: Mon, 13 Aug 2018 17:32:11 -0700 Subject: [PATCH 1/5] [RFC] Remove `broccoli-lint-eslint` and `ember-cli-eslint` --- active/0000-remove-ember-cli-eslint.md | 40 ++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) create mode 100644 active/0000-remove-ember-cli-eslint.md diff --git a/active/0000-remove-ember-cli-eslint.md b/active/0000-remove-ember-cli-eslint.md new file mode 100644 index 0000000..78f5a90 --- /dev/null +++ b/active/0000-remove-ember-cli-eslint.md @@ -0,0 +1,40 @@ +- Start Date: 2018-08-13 +- RFC PR: (leave this empty) + +# Summary + +Remove https://github.com/ember-cli/ember-cli-eslint from projects `ember-cli` generates. + +# Motivation + +1. Improve our build speed +2. Hacks required to support features such as [PR #122 + broccoli-lint-eslint](https://github.com/ember-cli/broccoli-lint-eslint/pull/122#discussion-diff-153937455R28) +3. Simplicity. `eslint` is common among JS stack, and integrations with editors + / precommit-hooks are ubiquitous. Removing this layer of abstraction will + simplify how `eslint` is used throughout `ember-cli`. + +# Detailed design + +1. Change blueprint to pull in `eslint` as opposed to `ember-cli-eslint` under + `devDependencies`. +2. Provide documentation on `eslint` and editor integration as well as precommit hooks + + +# How We Teach This + +Providing documentation should suffice. Also, going towards a much more explicit +path, this should become _easier_ to teach because of less abstractions. + +# Drawbacks + +1. No console warnings during builds +2. lint failures are no longer included in browser tests + +# Alternatives + +N/A + +# Unresolved questions + +N/A From 4f3a56c429b80dc6a2283fb2f29733ca262bb720 Mon Sep 17 00:00:00 2001 From: Sang Mercado Date: Mon, 27 Aug 2018 14:12:59 -0700 Subject: [PATCH 2/5] Adding summary and more context --- active/0000-remove-ember-cli-eslint.md | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/active/0000-remove-ember-cli-eslint.md b/active/0000-remove-ember-cli-eslint.md index 78f5a90..4d8066c 100644 --- a/active/0000-remove-ember-cli-eslint.md +++ b/active/0000-remove-ember-cli-eslint.md @@ -3,16 +3,28 @@ # Summary -Remove https://github.com/ember-cli/ember-cli-eslint from projects `ember-cli` generates. +Remove https://github.com/ember-cli/ember-cli-eslint from projects generated by +`ember-cli`. + +[ember-cli-eslint](https://github.com/ember-cli/ember-cli-eslint) is an addon +designed to show lint errors during test runs. Tooling around `eslint` has +improved enough where this feature may no longer be necessary. + +The Ember CLI core team have reached the consensus on removing +`ember-cli-eslint` as a dependency: + +https://github.com/ember-cli/ember-cli/issues/7975 # Motivation 1. Improve our build speed -2. Hacks required to support features such as [PR #122 - broccoli-lint-eslint](https://github.com/ember-cli/broccoli-lint-eslint/pull/122#discussion-diff-153937455R28) -3. Simplicity. `eslint` is common among JS stack, and integrations with editors +2. Simplicity. `eslint` is common among JS stack, and integrations with editors / precommit-hooks are ubiquitous. Removing this layer of abstraction will - simplify how `eslint` is used throughout `ember-cli`. + simplify how `eslint` is used throughout `ember-cli`. Most editors have + plugins available for `eslint`, and as long as the `.eslint.rc` is not + removed, we should still see the benefits of `eslint` in our Ember projects. +3. Hacks required to support features such as [PR #122 + broccoli-lint-eslint](https://github.com/ember-cli/broccoli-lint-eslint/pull/122#discussion-diff-153937455R28) # Detailed design From 36dce72b0c4382a93ac5202741225f113e63a39c Mon Sep 17 00:00:00 2001 From: Sang Mercado Date: Tue, 28 Aug 2018 10:23:31 -0700 Subject: [PATCH 3/5] Remove consensus --- active/0000-remove-ember-cli-eslint.md | 5 ----- 1 file changed, 5 deletions(-) diff --git a/active/0000-remove-ember-cli-eslint.md b/active/0000-remove-ember-cli-eslint.md index 4d8066c..a66297b 100644 --- a/active/0000-remove-ember-cli-eslint.md +++ b/active/0000-remove-ember-cli-eslint.md @@ -10,11 +10,6 @@ Remove https://github.com/ember-cli/ember-cli-eslint from projects generated by designed to show lint errors during test runs. Tooling around `eslint` has improved enough where this feature may no longer be necessary. -The Ember CLI core team have reached the consensus on removing -`ember-cli-eslint` as a dependency: - -https://github.com/ember-cli/ember-cli/issues/7975 - # Motivation 1. Improve our build speed From 6ae2c50a8b00fdc366abd70e03a3d1b0857f3b87 Mon Sep 17 00:00:00 2001 From: Sang Mercado Date: Sat, 8 Sep 2018 01:56:52 -0500 Subject: [PATCH 4/5] Be more descriptive about what this proposal is trying to achieve --- active/0000-remove-ember-cli-eslint.md | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/active/0000-remove-ember-cli-eslint.md b/active/0000-remove-ember-cli-eslint.md index a66297b..5385580 100644 --- a/active/0000-remove-ember-cli-eslint.md +++ b/active/0000-remove-ember-cli-eslint.md @@ -10,6 +10,18 @@ Remove https://github.com/ember-cli/ember-cli-eslint from projects generated by designed to show lint errors during test runs. Tooling around `eslint` has improved enough where this feature may no longer be necessary. +To be clear, the proposal is _not_ to remove linting in tests. It is to follow +the rest of JavaScript community and follow the standard tooling process. + +There are multiple ways to run `eslint`: + +1. Integration with editors +2. Utilize precommit hooks with `eslint` +3. Support a standard way to run `eslint` (such as `yarn lint:js`) + +We can also discuss configuring `testem` to automatically run `eslint` as part +of `yarn test` + # Motivation 1. Improve our build speed @@ -27,11 +39,13 @@ improved enough where this feature may no longer be necessary. `devDependencies`. 2. Provide documentation on `eslint` and editor integration as well as precommit hooks - # How We Teach This -Providing documentation should suffice. Also, going towards a much more explicit -path, this should become _easier_ to teach because of less abstractions. +Providing documentation regarding how to run linting should suffice as well as +documentation to editor integration. + +Deleting abstractions and going towards a explicit path, `eslint` within the +`ember-cli` ecosystem becomes _easier_ to teach. # Drawbacks @@ -40,7 +54,7 @@ path, this should become _easier_ to teach because of less abstractions. # Alternatives -N/A +1. Leave `ember-cli-eslint` alone # Unresolved questions From e9eb29f1fb0f01eb7b593aab3f9840995b5948a9 Mon Sep 17 00:00:00 2001 From: Sang Mercado Date: Fri, 26 Oct 2018 13:39:12 -0700 Subject: [PATCH 5/5] Add information about redefining `npm test` --- active/0000-remove-ember-cli-eslint.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/active/0000-remove-ember-cli-eslint.md b/active/0000-remove-ember-cli-eslint.md index 5385580..a3f9848 100644 --- a/active/0000-remove-ember-cli-eslint.md +++ b/active/0000-remove-ember-cli-eslint.md @@ -39,6 +39,20 @@ of `yarn test` `devDependencies`. 2. Provide documentation on `eslint` and editor integration as well as precommit hooks +Redefine `npm test` or `yarn test` (depending on whether the `--yarn` option was +used to create project) to + +``` +ember test && npm run lint:js && npm run lint:hbs +``` + +and + +``` +ember test && yarn lint:js && yarn lint:hbs +``` + + # How We Teach This Providing documentation regarding how to run linting should suffice as well as