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 3 commits
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
47 changes: 47 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,47 @@
- Start Date: 2018-08-13
- RFC PR: (leave this empty)

# Summary

Remove https://github.com/ember-cli/ember-cli-eslint from projects generated by
`ember-cli`.
Copy link

Choose a reason for hiding this comment

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

Are you proposing the removal of lint hooks completely from ember-cli, or just removing ember-cli-eslint as a default dependency of new projects?

Copy link
Author

Choose a reason for hiding this comment

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

As far as I understand, only reason why we need the lint hooks is to trigger logic within ember-cli-eslint. (I assumed they were tightly coupled since they were marked as private).

If that's not the case, we should leave the link hooks as is.

Copy link

Choose a reason for hiding this comment

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

There are some hooks beyond the ones you mentioned here, like lintTree that are used by a number of addons in the ecosystem.

But more broadly, broccoli-lint-eslint and ember-cli-eslint aren't dependencies of ember-cli itself, only of projects that the CLI generates. Given the phrasing here, I'm still a little unclear about whether you're proposing that Ember CLI stop supporting linters as part of the build, or just changing the default for new projects while leaving the ability for folks to opt in 🙂

Copy link
Author

Choose a reason for hiding this comment

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

I can see how that's unclear. I'll make some changes 👍 Thanks for the feedback @dfreeman


[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.

# Motivation

1. Improve our build speed
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`. 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

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