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

[sangm]

RENDERED

Initial draft

[dfreeman]

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?

[sangm]

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.

[dfreeman]

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 🙂

[sangm]

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

[jnfingerle]

Thanks for taking your time to write this RFC, nonetheless I'm against this.

Linting as part of the test is a big feature to enforce project standards. Editor integration and pre-commit hooks are great for developer convenience, but cannot replace this.

pre-receive hooks could replace this, but (a) aren't available on hosted (standard) github and (b) would mean that everybody has to do something by hand that ember could have done for them.

I do understand, that I could just add ember-cli-eslint by hand and I would do this for each and every project I'd be creating. But the default blueprint is supposed to be a good starting point, not a minimal one.

[sandstrom]

"No console warnings during builds" is a big drawback.

Will there be another, easy, way of getting them back if this RFC would be accepted?

[joukevandermaas]

I agree with @jnfingerle. I don't think the motivations listed are a good trade-off here for most projects. I think that "most projects do/don't want this" should be the main motivation for adding or removing things from the default template. I think most javascript developers would agree that linters are invaluable, and the best time to add a linter is at the start of a project.

That said, perhaps some more detailed explanation of the motivations could change my mind on the trade-offs involved. I have to admit that I don't understand the context for point 2 and 3 at all, and point 1 is not backed up with data.

[joshsearles]

I would be totally for this as an Optional feature. Having linting out of the box available I believe enforces a common standard and is a big help to users who don't use an IDE to code or even a second layer precaution to those new to Ember.

[jrjohnson]

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.

[sangm]

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

[rwjblue]

@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

[sangm]

@kellyselden / @rwjblue have deeper insights into how this decision may affect ember-cli: ember-cli/ember-cli#7975

[sangm]

As an example of how this affects ember-cli:

ember-cli/ember-cli-eslint#56 (comment)

[happycollision]

Anecdotally, I always check the box to ignore lint errors when I am running tests in the browser because I am in “make it work” mode. When I am refactoring I check for lint errors. But I appreciate having those warnings (and some explanations) in my IDE during all phases of writing my code.

So I guess I am saying that if removing ember-cli-eslint would still allow ember-specific lint messages in my IDE, I am fine with that. But, as a beginner, I am very happy that those messages appeared out of the box.

[jrjohnson]

@happycollision yes. I think that is missing from this RFC - that highlighting (at least in my IDE VS Code) is done by the standard eslint plugin. If I understand it correctly nothing Ember is required. I think this RFC should make it clear that what ember-cli-eslint does is to lint during test runs.

It is Eslint and your apps .eslint.rc file that do the actual linting and those will remain.

[sangm]

Updated @jrjohnson

[jnfingerle]

1. I don't see what place a consensus in the core team might have in this RFC. If there is already a consensus, they should just do it, so we don't have to use up our time on this RFC. If, on the other hand (and as I understood), the core team is interested in the community's view and weighs in the arguments in a RFC discussion, it's premature to talk about a consensus and at best an argumentum ad verecundiam (argument from authority). The RFC should rely on it's own merits, not on real or supposed approval from a special subset of the community. That said, I'm aware that this might still be the consensus in the end. But having it written into the RFC before the formal decision is made is leaving more than a bitter taste on my end.

2. "Drawback" is still missing, that ember-cli-eslint is the easiest way to have automated builds fail on eslint errors and warning, regardless of the tool used for building.

Obviously you might have different views on the matter if a failed lint should be treated the same as a failed test. Since eslint contains many rules that aren't only about appearance but to enforce coding habits that are less error prone I more than tend to that it should be treated equally.

[sangm]

@jnfingerle (1) is a good point. I'll take it out

[buschtoens]

When this RFC was originally submitted I leaned towards 👎 , but after reading through ember-cli/ember-cli-eslint#235, this changed to a strong 👍 .

As long as linting is still part of the default CI, as requested here, I am very much in favor of this. Providing an optional good and fast default pre-commit / pre-push hook would also be super sweet!

[rwjblue]

I completely agree with this RFC and think it is without a doubt what we should do going forward.

See ember-cli/ember-cli-eslint#235 for yet another example of the issues that we face if we try to "stay in the middle".

[rwjblue]

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

[rwjblue]

@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

[rwjblue]

@jnfingerle

Linting as part of the test is a big feature to enforce project standards.

I totally agree that linting is super important, and that projects should enforce linting standards before merging any changes. This RFC doesn't make any changes to ember-cli's stance on that, it doesn't make it more difficult to see linting failures (in fact with yarn lint:js / yarn lint:hbs I think it is actually much easier to reason about), and it makes the Ember ecosystem "just another consumer" of standard JS tooling.

@sandstrom

"No console warnings during builds" is a big drawback.

I agree, @sangm can you add that to the list of drawbacks? FWIW, I do think there may be other ways to solve this, and I also think that even with these changes we still get direct "in your face" feedback that you have linting failures (right there in your editor without having to flip around to the terminal)...

@happycollision

Anecdotally, I always check the box to ignore lint errors when I am running tests in the browser because I am in “make it work” mode. When I am refactoring I check for lint errors. But I appreciate having those warnings (and some explanations) in my IDE during all phases of writing my code.

Exactly! I also do exactly the same thing (disabling linting while running QUnit tests). When I'm ready to commit things, I either run yarn lint:js or leverage precommit hooks (though I am much less in favor of precommit hooks 😛). The key here is that we aren't talking "removing linting" and the feedback loop is really best when directly in editor anyways...

IMHO, this is absolutely important and we should move forward...

Note: I happened to do my initial review of this RFC during an “issue triage” stream I was doing, you can check that out here

---

I look forward to hearing folks comments / thoughts, please keep the conversation going... 😸

[webark]

In a similar vein to adding a lint command to travis, for those that want the linting every time tests are run, how hard is it to add a yarn test or equivalent to the testem config to fail the tests when they are run as normal?

[webark]

Also, i’d suggest changing the name to something more like “replace ember-cli-eslint with the standard eslint package” or something. I’m wondering if some are thrown off by the initial name of the PR, and feel that it is implying linting is going to be removed from the default package completely.

[sangm]

@webark Done. Thanks for the suggestion 👍

[rwjblue]

how hard is it to add a yarn test or equivalent to the testem config to fail the tests when they are run as normal?

Ya, should be possible assuming we can make the test script in package.json essentially be yarn lint:js && yarn lint:hbs && ember test in a platform compatible way (I’m not sure the best way to do this that works on both *nix and win).

I do like the idea of yarn test/ npm test still being the final pass/fail...

[webark]

I do like the idea of yarn test/ npm test still being the final pass/fail...

Oh sure, but i’d say there is a community interest that optionally a lint fail would fail the build. I don’t think they have to be seems as “tests” like the do know, but it would be nice if you could configure testem to run the lint command, then if there where failures, run the tests, and fail.

If testem doesn’t have that functionality.. :/ then that’s too bad, but a feature issue could be raised over there.

[kellyselden]

The RFC doesn't specify, but anyone without the --yarn option will get npm run commands in their project instead of yarn.

[jfelchner]

@rwjblue pre-commit hooks are amazing. Fight me. 😂❤️

[rwjblue]

@jfelchner - another place, another time 😜

[rwjblue]

@sangm - ember-cli supports projects using either, we attempt to detect which is right (based on the --yarn flag, presence of yarn.lock, etc). For the purposes of the RFC we would basically be doing what @jnfingerle and @kellyselden mentioned (using your package manager of choice).

[rwjblue]

After discussing with the rest of the ember-cli team at todays meeting we are all in favor of moving forward. As part of the implementation work we would like to clearly document how to setup testem.js to properly run linting (both eslint and template linting) during ember test --server and at least link to it from an inline comment in testem.js (possibly even include it automatically?).

[rwjblue]

Thank you all for the awesome conversation. I truly believe that we came up with a great solution together.