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

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

Merged
rwjblue merged 5 commits into from
Nov 1, 2018
Merged

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

Changes from all 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
75 changes: 75 additions & 0 deletions active/0000-remove-ember-cli-eslint.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
- 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`.

@dfreemandfreemanAug 14, 2018

Copy link
Copy Markdown

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?

@sangmsangmAug 14, 2018

Copy link
Copy Markdown
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.

@dfreemandfreemanAug 14, 2018

Copy link
Copy Markdown

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 🙂

@sangmsangmAug 15, 2018

Copy link
Copy Markdown
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.

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

@jrjohnsonjrjohnsonAug 16, 2018

Copy link
Copy Markdown

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.

@sangmsangmAug 27, 2018

Copy link
Copy Markdown
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!

@rwjbluerwjblueSep 6, 2018

Copy link
Copy Markdown
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

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
documentation to editor integration.

Deleting abstractions and going towards a explicit path, `eslint` within the
`ember-cli` ecosystem becomes _easier_ to teach.

# Drawbacks

1. No console warnings during builds
2. lint failures are no longer included in browser tests

# Alternatives

1. Leave `ember-cli-eslint` alone

# Unresolved questions

N/A