Use `eslint` and `ember-template-lint` directly (no longer lint during builds/rebuilds by default) by dcyriller · Pull Request #9009 · ember-cli/ember-cli

dcyriller · 2020-01-10T12:54:01Z

This is another attempt at implementing https://github.com/emberjs/rfcs/blob/master/text/0121-remove-ember-cli-eslint.md . It would superseed #8219 if you agree @kellyselden .

I am ready to write documentation for this. I'm not sure where though.

Drawbacks

This implementation has two drawbacks listed in the RFC:

no console warnings during builds
lint failures are no longer included in browser tests

For users wishing these features back, the way to go would be to manually install ember-cli-template-lint and ember-cli-eslint.

Links

Related discussions: ember-template-lint/ember-cli-template-lint#557, #8982 and many others 😄
RFC: https://github.com/emberjs/rfcs/blob/master/text/0121-remove-ember-cli-eslint.md
a WIP PR for this RFC: #8219

rwjblue · 2020-02-10T16:01:41Z

In general, this is looking good. The main thing remaining is to ensure that npm test / yarn test still fail (properly) if there are linting failures.

Turbo87 · 2020-02-12T18:02:13Z

The main thing remaining is to ensure that npm test / yarn test still fail (properly) if there are linting failures.

FWIW I don't think this needs to be a high priority. I usually use yarn test to run the tests and yarn lint to run all the linting checks and CI just runs both commands instead of relying on yarn test running both.

our CI template currently does run both, yarn lint and yarn test, so making yarn test do linting too, would mean that linting is now running twice 🤔

rwjblue · 2020-02-13T20:18:41Z

Rebased and fixed conflicts with other changes on master.

kellyselden · 2020-02-14T10:33:17Z

@rwjblue It looks like you lost eslint in the rebase.

rwjblue · 2020-02-14T14:39:56Z

Thank you @kellyselden! Fixed.

rwjblue · 2020-02-14T20:35:25Z

CI is failing here because ember-template-lint@2.0.0 isn’t released yet.

dcyriller · 2020-02-17T17:04:27Z

Indeed, sorry @rwjblue. I was on vacations. I rolled back to ember-template-lint v1 as v2 is still in beta. In theory, we should be able to upgrade template-lint to v2 by reverting 7d600b1.

.. and ember-template-lint Implements RFC ember-cli/rfcs#121

As v2 is in beta at the moment

ESLint is not run anymore by the command `ember test` in a brand new app (or addon). Indeed, ember-cli-eslint is gone now.

dcyriller · 2020-02-17T18:46:22Z

Rebased and fix the last CI failure in tests/acceptance/smoke-test-slow.js (that file was changed on master).

mehulkar · 2020-02-18T19:10:54Z

The main thing remaining is to ensure that npm test / yarn test still fail (properly) if there are linting failures.

FYI, if the blueprint scripts don't include run the linter on npm test, new users will have no feedback on failures. Especially for the simple a11y lints, I think it's valuable to have those to nudge people in the right direction. Seems like a simple && change in the test script would fix this?

kellyselden · 2020-02-18T19:45:28Z

My PR is a more complete version of this if you think I should revive/rebase it? #8219

dcyriller · 2020-02-18T21:14:51Z

if the blueprint scripts don't include run the linter on npm test, new users will have no feedback on failures.

I might be biaised on this. But to me, linting and testing look better run in parallel than sequentially. IMHO new users should be taught to run them in parallel in CI.

Also, we could document editor integrations (and also git hooks) to help users catch lint errors / warnings early.

mehulkar · 2020-02-18T21:47:35Z

Parallel is better. We use `concurrently` for this but that’s probably not something to put in the blueprint

rwjblue · 2020-02-23T01:17:19Z

As part of merging RFC#121 we collectively agreed that the normal Node ecosystem conventions should apply. This means that we must ensure that npm test (or yarn test) fails when we consider the repository to be in a "bad state".

Prior to the changes proposed in the RFC, this was true (kinda? 😃) because ember-cli-eslint and ember-cli-template-lint would add errors to the ember test run. The pre-RFC121 setup fell down in a
few important (and IMHO fatal) ways:

It was only possible to lint files that were part of the application build. This meant that issues in files in other directories (e.g. index.js, ember-cli-build.js, etc) would not fail npm test / yarn test.
For addon's, important compatibility tests (ember-try scenarios) were never ran. If an addon didn't use the generated .travis.yml setup they would never have their compatibility tests ran 😱.

I've just pushed a commit that does a few specific things to address the concerns above, and make it possible to deliver on the promises that we made in order to land RFC121:

Add a lint script to projects that runs both lint:js and lint:hbs (in parallel) aggregating the results.
Add a test:ember script that runs ember test (previously was the test script).
Change the test script to run lint:js, lint:hbs, and test:ember scripts
For addons, rename the test:each script to test:ember-try (test:each seemed too ambiguous) and ensure test script runs this as well.
Simplify the .travis.yml configuration now that npm test actually does the right thing 😸

rwjblue · 2020-02-23T01:21:28Z

if the blueprint scripts don't include run the linter on npm test, new users will have no feedback on failures.

@mehulkar is absolutely right here! This is both fatal (for the change) and failing to account for this constraint would be intentionally choosing to deviate from the agreements we made in landing the RFC (see the detailed design).

My PR is a more complete version of this if you think I should revive/rebase it? #8219

@kellyselden - Your PR does indeed do much more than this one, but this one meets the requirements of the original RFC and sets the stage for us to actually have the conversations on the more controversial parts of your PR. I'd rather move forward with smaller changes here, then do follow up PR's to continue iterate...

rwjblue · 2020-02-23T01:22:32Z

our CI template currently does run both, yarn lint and yarn test, so making yarn test do linting too, would mean that linting is now running twice 🤔

@Turbo87 - Only if we screw up and forget to update both places 😜

rwjblue · 2020-02-23T01:28:56Z

blueprints/app/files/package.json

  },
  "scripts": {
    "build": "ember build --environment=production",
+    "lint": "npm-run-all --aggregate-output --continue-on-error --parallel lint:*",


Checkout the docs on the npm-run-all command, but this is basically running lint:js and lint:hbs in parallel and ensuring the console output is aggregated (so they don't get interleaved).

rwjblue · 2020-02-23T01:30:50Z

blueprints/app/files/package.json

    "eslint-plugin-ember": "^7.7.2",
    "eslint-plugin-node": "^11.0.0",
    "loader.js": "^4.7.0",
+    "npm-run-all": "^4.1.5",


To make it easier for others to find, npm-run-all is a very popular (~ 4M downloads/month) package made to do exactly the thing we are trying to do (with some neat features like glob support).

https://www.npmjs.com/package/npm-run-all

kategengler · 2020-02-23T03:26:07Z

👍 I wonder if test:ember-try is also ambiguous? My thought is something like test:ember-compatibility?

Turbo87

as I mentioned before, I personally would prefer to only run ember test for the test script 🤷‍♂

Turbo87 · 2020-02-23T06:43:25Z

tests/fixtures/addon/npm/package.json

-    "test:all": "ember try:each"
+    "test": "npm-run-all lint:* test:*",
+    "test:ember": "ember test",
+    "test:ember-try": "ember try:each"


that means yarn test will now run ember try too? we explicitly changed it a while ago to not do that because running it takes forever and canceling it leaves your repo in a somewhat weird state.

We should fix the cancellation issues (regardless of choice here), but IMHO this is a feature not a bug. All too many times I’ve been bitten by addons that are not using the default .travis.yml setup (using something like circle, GH actions, or other node aware tooling) that assume npm test means “good to go”.

blueprints/app/files/package.json

tests/fixtures/addon/npm/.travis.yml

blueprints/addon/files/.travis.yml

rwjblue · 2020-02-23T16:32:47Z

👍 I wonder if test:ember-try is also ambiguous? My thought is something like test:ember-compatibility?

Done!

blueprints/addon/files/.travis.yml

kellyselden · 2020-02-23T16:40:26Z

tests/fixtures/addon/npm/package.json

-    "test:all": "ember try:each"
+    "test": "npm-run-all lint:* test:*",
+    "test:ember": "ember test",
+    "test:ember-compatibility": "ember try:each"


test:ember-compat to make it easier to type?

I don't know that folks will type it much? I'd prefer to avoid the abbreviation

dcyriller · 2020-02-23T16:51:32Z

I've just pushed a commit that does a few specific things to address the concerns above, and make it possible to deliver on the promises that we made in order to land RFC121

Thank you @rwjblue!

As part of merging RFC#121 we collectively agreed that the normal Node ecosystem conventions should apply. This means that we **must** ensure that `npm test` (or `yarn test`) fails when we consider the repository to be in a "bad state". Prior to the changes proposed in the RFC, this was true (mostly :smiley: ) because ember-cli-eslint and ember-cli-template-lint would add errors to the `ember test` run. The pre-RFC121 setup fell down in a few (IMHO important and fatal) ways: * It was only possible to lint files that were part of the application build. This meant that issues in files in other directories (e.g. `index.js`, `ember-cli-build.js`, etc) would *not* fail `npm test` / `yarn test`. * For addon's, important compatibility tests (ember-try scenarios) were never ran. If an addon didn't use the generated `.travis.yml` setup they would _never_ have their compatibility tests ran :scream:. This commit does a few specific things to address the concerns above, and make it possible to deliver on the promises that we made in order to land RFC121: * Add a `lint` script to projects that runs both `lint:js` and `lint:hbs` (in parallel) aggregating the results. * Add a `test:ember` script that runs `ember test` (previously was the test script). * Change the `test` script to run `lint:js`, `lint:hbs`, and `test:ember` scripts * For addons, rename the `test:each` script to `test:ember-compatibility` (`test:each` seemed too ambiguous) and ensure `test` script runs this as well. * Simplify the `.travis.yml` configuration now that `npm test` actually does the right thing :smile_cat:

ember-cli/ember-cli#9009 changed the yarn test script such that it is not directly calling `ember test`. Since `--path` is an option we want to pass to `ember test`, we should call it directly.

ember-cli/ember-cli#9009 changed the yarn test script such that it is not directly calling `ember test`. Since `--path` is an option we want to pass to `ember test`, we should call it directly. --- Commit: ember-learn/super-rentals-tutorial@64f1714 Script: https://github.com/ember-learn/super-rentals-tutorial/blob/64f17140a8ddb38627d481453427942fa6290371/.github/workflows/build.yml Logs: https://github.com/ember-learn/super-rentals-tutorial/commit/64f17140a8ddb38627d481453427942fa6290371/checks

BlueRaja · 2020-06-18T19:55:14Z

lint failures are no longer included in browser tests

Yikes, this drawback should've been a blocker.

If the main concern was build time, couldn't you skip linting during the build, while still running lint in the browser tests??

rwjblue · 2020-06-18T20:11:01Z

couldn't you skip linting during the build, while still running lint in the browser tests??

No. How do those tests end up in the browser? Because we lint them in the build, and copy the output into the dist/assets/test.js.

dcyriller mentioned this pull request Jan 10, 2020

Prepare a v2 ember-template-lint/ember-cli-template-lint#560

Closed

dcyriller mentioned this pull request Feb 4, 2020

Allow ast-plugin to run in parallel ember-template-lint/ember-cli-template-lint#579

Merged

rwjblue mentioned this pull request Feb 14, 2020

Support eslint 6 ember-cli/ember-cli-eslint#460

Closed

dcyriller added 3 commits February 17, 2020 19:43

Replace ember-cli-{eslint,template-lint} by eslint..

2d06e0c

.. and ember-template-lint Implements RFC ember-cli/rfcs#121

Use ember-template-lint v1

c0c0da3

As v2 is in beta at the moment

Update smoke test

6de31bf

ESLint is not run anymore by the command `ember test` in a brand new app (or addon). Indeed, ember-cli-eslint is gone now.

rwjblue mentioned this pull request Feb 21, 2020

Don't mention ember-cli-template-lint in README ember-template-lint/ember-template-lint#1133

Merged

Update to ember-template-lint@2.0.0.

fa32b58

rwjblue reviewed Feb 23, 2020

View reviewed changes

rwjblue approved these changes Feb 23, 2020

View reviewed changes

Turbo87 reviewed Feb 23, 2020

View reviewed changes

kellyselden reviewed Feb 23, 2020

View reviewed changes

blueprints/app/files/package.json Show resolved Hide resolved

tests/fixtures/addon/npm/.travis.yml Outdated Show resolved Hide resolved

kellyselden reviewed Feb 23, 2020

View reviewed changes

blueprints/addon/files/.travis.yml Outdated Show resolved Hide resolved

kellyselden reviewed Feb 23, 2020

View reviewed changes

blueprints/addon/files/.travis.yml Outdated Show resolved Hide resolved

kellyselden reviewed Feb 23, 2020

View reviewed changes

kellyselden approved these changes Feb 23, 2020

View reviewed changes

rwjblue changed the title ~~RFC-121: Remove ember-cli-eslint and ember-cli-template-lint from app blueprints~~ Use eslint and ember-template-lint directly (no longer lint during builds/rebuilds by default) Feb 24, 2020

rwjblue merged commit 1dd8f4b into ember-cli:master Feb 24, 2020

rwjblue mentioned this pull request Feb 24, 2020

WIP: Swap ember-cli-eslint for ESLint #8219

Closed

dcyriller deleted the rfc-121 branch February 24, 2020 14:47

chancancode mentioned this pull request Feb 26, 2020

Run ember test directly when passing --path ember-learn/super-rentals-tutorial#138

Merged

BarryThePenguin mentioned this pull request Mar 12, 2020

Running npm test with additional arguments #9084

Closed

jaydgruber mentioned this pull request Apr 6, 2020

chore: Angle Brackets Codemod on app/template/accounts/ fossasia/open-event-frontend#4301

Merged

5 tasks

snyk-bot mentioned this pull request Apr 9, 2020

[Snyk] Upgrade ember-cli from 3.16.1 to 3.17.0 nodeshift-starters/opossum-examples#32

Merged

backspace mentioned this pull request Jul 7, 2020

UI: Update template linting and fix missed curly invocations hashicorp/nomad#8382

Merged

mehulkar mentioned this pull request Jul 20, 2020

Remove ember-default ember-try scenario #9280

Merged

patocallaghan mentioned this pull request Jan 12, 2021

Migrated CI from Travis to GitHub Actions gothinkster/ember-realworld#117

Merged

This was referenced Jan 18, 2022

Add deprecation notice to README ember-cli/ember-cli-eslint#597

Merged

Add deprecation notice to README billybonks/ember-cli-stylelint#483

Open

kategengler mentioned this pull request Mar 10, 2025

Update the test script so it only runs the ember tests #10663

Closed

Conversation

dcyriller commented Jan 10, 2020

Drawbacks

Links

Uh oh!

rwjblue commented Feb 10, 2020

Uh oh!

Turbo87 commented Feb 12, 2020

Uh oh!

rwjblue commented Feb 13, 2020

Uh oh!

kellyselden commented Feb 14, 2020

Uh oh!

rwjblue commented Feb 14, 2020

Uh oh!

rwjblue commented Feb 14, 2020

Uh oh!

dcyriller commented Feb 17, 2020

Uh oh!

dcyriller commented Feb 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mehulkar commented Feb 18, 2020

Uh oh!

kellyselden commented Feb 18, 2020

Uh oh!

dcyriller commented Feb 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mehulkar commented Feb 18, 2020 via email • edited by rwjblue Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rwjblue commented Feb 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rwjblue commented Feb 23, 2020

Uh oh!

rwjblue commented Feb 23, 2020

Uh oh!

rwjblue Feb 23, 2020

Choose a reason for hiding this comment

Uh oh!

rwjblue Feb 23, 2020

Choose a reason for hiding this comment

Uh oh!

kategengler commented Feb 23, 2020

Uh oh!

Turbo87 left a comment

Choose a reason for hiding this comment

Uh oh!

Turbo87 Feb 23, 2020

Choose a reason for hiding this comment

Uh oh!

rwjblue Feb 23, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rwjblue commented Feb 23, 2020

Uh oh!

Uh oh!

kellyselden Feb 23, 2020

Choose a reason for hiding this comment

Uh oh!

rwjblue Feb 23, 2020

Choose a reason for hiding this comment

Uh oh!

dcyriller commented Feb 23, 2020

Uh oh!

BlueRaja commented Jun 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rwjblue commented Jun 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

dcyriller commented Feb 17, 2020 •

edited

Loading

dcyriller commented Feb 18, 2020 •

edited

Loading

mehulkar commented Feb 18, 2020 via email •

edited by rwjblue

Loading

rwjblue commented Feb 23, 2020 •

edited

Loading

BlueRaja commented Jun 18, 2020 •

edited

Loading