Skip to content
This repository has been archived by the owner on Jan 20, 2019. It is now read-only.

[RFC] Replace ember-cli-eslint with the standard eslint #121

Merged
merged 5 commits into from
Nov 1, 2018
Merged

[RFC] Replace ember-cli-eslint with the standard eslint #121

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1c50958
[RFC] Remove `broccoli-lint-eslint` and `ember-cli-eslint`
Aug 14, 2018
4f3a56c
Adding summary and more context
Aug 27, 2018
36dce72
Remove consensus
Aug 28, 2018
6ae2c50
Be more descriptive about what this proposal is trying to achieve
Sep 8, 2018
e9eb29f
Add information about redefining `npm test`
Oct 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions active/0000-remove-ember-cli-eslint.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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

Copy link

Choose a reason for hiding this comment

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

Maybe:
3. Add to the default Travis blueprint:

script:
  - yarn lint:js
  - yarn test

To make it clear that linting is still running (and still part of the test process) it just doesn't run along side the other tests.

Copy link
Author

Choose a reason for hiding this comment

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

I think this is a good idea. Thanks @jrjohnson!

Copy link
Member

Choose a reason for hiding this comment

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

@jrjohnson - You are totally correct! So much so, that its already what we do!

https://github.com/ember-cli/ember-new-output/blob/v3.4.1/.travis.yml#L25-L27


# 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
Copy link
Member

Choose a reason for hiding this comment

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

We at least mention the alternative of: "do nothing" (not that I think that is a good thing to do...)


# Unresolved questions

N/A