WIP: Swap `ember-cli-eslint` for ESLint by kellyselden · Pull Request #8219 · ember-cli/ember-cli

kellyselden · 2018-11-28T21:11:46Z

I already started doing this at work, so I want to get it upstream.

kellyselden · 2018-11-28T21:15:25Z

blueprints/app/files/README.md


-* `ember test`
-* `ember test --server`
+* `<% if (yarn) { %>yarn test<% } else { %>npm test<% } %>`


We already have linting mentioned below, so it's a bit redundant to mention the command that runs linting as well. On the other hand, how are people going to learn that ember test no longer lints, and that you should be running npm test to get the old behavior?

Also, we now have a weird mix of npm-scripts wrapped commands and native ember-cli commands. We may need learning team help on resolving this confusion. cc @jenweber @NullVoxPopuli @kennethlarsen

maybe, just concat the commands together (at least for now?)

like,

yarn lint:js && yarn test

or, we split it out into different travis jobs?

You mean yarn lint:js && ember test?

It is already split in travis (or should be).

You mean yarn lint:js && ember test?

idk, I prefix everything with yarn, so I know I'm using the local version

It is already split in travis (or should be).

#biwinning

Perhaps the test output could indicate that linting is not included. For example, it could say afterwards "to run linting, use npm test." I will have to think about this some.

Beginners aside, this is a tough question because I imagine that many people just do ember test in their CI. I don't use Travis and I'm pretty sure that I just run ember test in CI.

kellyselden · 2018-11-28T21:16:10Z

blueprints/app/files/package.json

@@ -15,7 +15,7 @@
    "lint:hbs": "ember-template-lint .",
    "lint:js": "eslint .",
    "start": "ember serve",


Should we include this in the start and build commands (and test:all) as well?

for least suprise, I think it'd make sense to add it.
But, I think just mentioning it in the release notes would be ok if we were to stop running linting before everything.
idk 🤷‍♂️

kellyselden · 2018-11-28T21:18:25Z

blueprints/app/files/package.json

    "lint:js": "eslint .",
    "start": "ember serve",
-    "test": "ember test"
+    "test": "<% if (yarn) { %>yarn lint:hbs<% } else { %>npm run lint:hbs<% } %> && ember test"


We could also utilize "pretest" as well. I'm not sure of the advantages of each. This is essentially equivalent:

"pretest": "npm run lint:js", "test": "ember test"

kellyselden · 2018-11-28T21:21:49Z

blueprints/app/files/package.json

    "lint:js": "eslint .",
    "start": "ember serve",
-    "test": "ember test"
+    "test": "<% if (yarn) { %>yarn lint:js<% } else { %>npm run lint:js<% } %> && ember test"


I left off npm run lint:hbs because there is currently no way that I know to take template linting out of ember test. Once that is possible, we can add it here. @rwjblue correct me if I'm wrong.

You can remove ember-cli-template-lint and replace with ember-template-lint directly. This will keep Test'em from linting the hbs files, but you can still yarn lint:hbs with no issues.

kellyselden · 2018-11-28T21:22:46Z

cc @sangm

NullVoxPopuli · 2018-11-28T21:29:14Z

blueprints/module-unification-app/files/README.md

-* `ember test`
-* `ember test --server`
+* `<% if (yarn) { %>yarn test<% } else { %>npm test<% } %>`
+* `<% if (yarn) { %>yarn test --server<% } else { %>npm test -- --server<% } %>`


kellyselden · 2018-11-28T22:25:47Z

blueprints/app/files/README.md

-* `ember test`
-* `ember test --server`
+* `<% if (yarn) { %>yarn test<% } else { %>npm test<% } %>`
+* `<% if (yarn) { %>yarn test --server<% } else { %>npm test -- --server<% } %>`


@john-kurkowski just pointed out to me that yarn test --server is misleading. One might assume the linting part is watching the build too. This might be a job for something like https://www.npmjs.com/package/eslint-watch? cc @Turbo87 I know you were talking about something like this at one point.

Right, I do like 1 SPOT to look for all errors in all files, continuously. Today, that's the launched Testem UI. It reminds me of things my editor doesn't, because I switched away from the file with errors.

Running another watch process is an option, sure.

Maybe a commit hook would cover my edge case.

I don't think a commit hook would solve your live updating needs, that's like a "final check once you're all done", right? not a "while you work" solution.

onlymejosh · 2019-02-14T09:31:43Z

From reading the RFC it looks like there was a discussion about adding the documentation for setting up testem.js with eslint.

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)

This should be a requirement of this PR as without it, it will completely change the workflow of some developers. The testem ui is basically my one-stop-shop for errors, I could add another step but it feels like going backwards rather than forwards. I don't want to have to rely on CI to tell me about linting issues and precommit hooks are a drag.

kellyselden · 2019-03-08T15:29:16Z

@onlymejosh I added the testem hook before_tests to run linting to keep the conversation going.

@rwjblue Was this what you were thinking?

onlymejosh · 2019-03-08T16:13:48Z

If there is a linting issue would this stop the rest of the tests running? If so this is still a backwards change in my eyes. It should be the same as it currently is, linting issues should appear in the list of failures.

rwjblue · 2019-03-08T16:18:31Z

If so this is still a backwards change in my eyes

I'm sorry you feel this way.

It should be the same as it currently is, linting issues should appear in the list of failures.

Simply put, thats not going to happen. The RFC (and comment thread) specifically explain the rational, and when implementation is completed a newly generated app will not have the setup you are talking about. However, it is absolutely still a thing you can choose to opt in to (by installing ember-cli-eslint in your particular application).

onlymejosh · 2019-03-08T16:24:53Z

Ah ok so the solution is to use ember-cli-eslint

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?).

Is this what you meant by documenting? I guess I was confused by that comment. As I took it as it would act the same / similarly but it looks like that is not the case?

sangm · 2019-06-04T08:18:30Z

Any update on this?

rwjblue · 2020-02-24T14:30:43Z

The basic parts of this were by #9009,

I'm going to close this specific PR as rebasing it would be a nightmare (removal of MU, landing of an alternate approach in #9009, etc), but I'd love to see the other conversations that were ongoing here in smaller PRs focused on each topic.

kellyselden commented Nov 28, 2018

View reviewed changes

kellyselden force-pushed the ember-cli-eslint branch from c69c492 to 546a9cd Compare November 28, 2018 21:20

kellyselden commented Nov 28, 2018

View reviewed changes

NullVoxPopuli reviewed Nov 28, 2018

View reviewed changes

kellyselden force-pushed the ember-cli-eslint branch 2 times, most recently from ff79c38 to 89a3a7a Compare November 28, 2018 21:48

kellyselden commented Nov 28, 2018

View reviewed changes

kellyselden force-pushed the ember-cli-eslint branch from 89a3a7a to f743166 Compare November 28, 2018 22:30

kellyselden added the Work in Progress label Nov 28, 2018

kellyselden changed the title ~~WIP: swap ember-cli-eslint for eslint~~ swap ember-cli-eslint for eslint Nov 28, 2018

Turbo87 changed the title ~~swap ember-cli-eslint for eslint~~ WIP: Swap ember-cli-eslint for ESLint Nov 29, 2018

Turbo87 removed the Work in Progress label Nov 29, 2018

This was referenced Nov 30, 2018

Temporarily update blueprint to reduce friction for contributors #8085

Closed

Remove ember-cli-eslint as a dependency #7975

Closed

jayjayjpg mentioned this pull request Dec 3, 2018

The Ember Times - Issue No. 76 emberjs/website#3714

Merged

13 tasks

kellyselden force-pushed the ember-cli-eslint branch from f743166 to 49309aa Compare December 3, 2018 20:32

kellyselden force-pushed the ember-cli-eslint branch from 49309aa to 0965514 Compare February 3, 2019 10:31

swap ember-cli-eslint for eslint

ec9a18b

kellyselden force-pushed the ember-cli-eslint branch from 0965514 to ec9a18b Compare March 8, 2019 14:49

run linting in testem before_tests

88bdbbd

dcyriller mentioned this pull request Jan 10, 2020

Use eslint and ember-template-lint directly (no longer lint during builds/rebuilds by default) #9009

Merged

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 closed this Feb 24, 2020

Conversation

kellyselden commented Nov 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

happycollision Jan 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kellyselden commented Nov 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

onlymejosh commented Feb 14, 2019

Uh oh!

kellyselden commented Mar 8, 2019

Uh oh!

onlymejosh commented Mar 8, 2019

Uh oh!

rwjblue commented Mar 8, 2019

Uh oh!

onlymejosh commented Mar 8, 2019

Uh oh!

sangm commented Jun 4, 2019

Uh oh!

rwjblue commented Feb 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

happycollision Jan 3, 2020 •

edited

Loading