Replace JSHint/JSCS with ESLint #163

Turbo87 · 2016-06-05T13:41:09Z

This PR enables emberjs/ember.js#13549 to be merged by replacing JSHint and JSCS with ESLint.

Resolves #161

/cc @rwjblue @stefanpenner

Turbo87 · 2016-06-05T16:15:55Z

this doesn't seem to work properly yet and I'm not quite sure why...

instructions:

check out the branch from ESLint ember.js#13549
install the branch from this PR into it
produce a lint issue in the ember.js repo
run ember test --server
tests should fail, but they seem to succeed

@BrianSipple any ideas?

stefanpenner · 2016-06-06T18:15:39Z

i like were this is going!

Turbo87 · 2016-06-09T10:39:17Z

lib/eslint-tree.js

+    testGenerator: testGenerator,
+  });
+
+  esLintTree.targetExtension = 'eslint.js';


It looks like this line was causing the issues. If the default lint-test.js extension is used instead it works properly...

Turbo87 · 2016-06-09T11:03:48Z

@rwjblue @stefanpenner this should be ready for merging now. you can test this with the instructions above.

Turbo87 · 2016-06-10T16:11:55Z

TravisCI failures seem unrelated to this PR

stefanpenner · 2016-06-10T16:15:53Z

I'll try to find time over the next few days, feel free to bug me if i dont.

Turbo87 · 2016-06-18T09:49:53Z

@stefanpenner ping ;)

Turbo87 · 2016-06-21T07:52:40Z

🐛

stefanpenner · 2016-06-22T17:11:08Z

👀

stefanpenner · 2016-06-22T17:42:16Z

running this i get

File: ember-metal/tests/features_test.js
The Broccoli Plugin: [Babel] failed with:
Error: ember-metal/tests/features_test.js: [BABEL] ember-metal/tests/features_test.js: An unknown feature 'fred'' was encountered
    at Logger.error (/Users/stefanpenner/src/ember.js/node_modules/babel-core/lib/transformation/file/logger.js:58:11)
    at NodePath.babel.Transformer.CallExpression (/Users/stefanpenner/src/ember.js/node_modules/babel-plugin-feature-flags/index.js:42:22)
    at NodePath.call (/Users/stefanpenner/src/ember.js/node_modules/babel-core/lib/traversal/path/context.js:56:28)
    at NodePath.visit (/Users/stefanpenner/src/ember.js/node_modules/babel-core/lib/traversal/path/context.js:90:8)
    at TraversalContext.visitMultiple (/Users/stefanpenner/src/ember.js/node_modules/babel-core/lib/traversal/context.js:108:16)
    at TraversalContext.visit (/Users/stefanpenner/src/ember.js/node_modules/babel-core/lib/traversal/context.js:146:19)
    at Function.traverse.node (/Users/stefanpenner/src/ember.js/node_modules/babel-core/lib/traversal/index.js:76:17)
    at NodePath.visit (/Users/stefanpenner/src/ember.js/node_modules/babel-core/lib/traversal/path/context.js:107:26)
    at TraversalContext.visitSingle (/Users/stefanpenner/src/ember.js/node_modules/babel-core/lib/traversal/context.js:132:12)
    at TraversalContext.visit (/Users/stefanpenner/src/ember.js/node_modules/babel-core/lib/traversal/context.js:148:19)
    at Function.traverse.node (/Users/stefanpenner/src/ember.js/node_modules/babel-core/lib/traversal/index.js:76:17)
    at NodePath.visit (/Users/stefanpenner/src/ember.js/node_modules/babel-core/lib/traversal/path/context.js:107:26)
    at TraversalContext.visitMultiple (/Users/stefanpenner/src/ember.js/node_modules/babel-core/lib/traversal/context.js:108:16)
    at TraversalContext.visit (/Users/stefanpenner/src/ember.js/node_modules/babel-core/lib/traversal/context.js:146:19)
    at Function.traverse.node (/Users/stefanpenner/src/ember.js/node_modules/babel-core/lib/traversal/index.js:76:17)
    at NodePath.visit (/Users/stefanpenner/src/ember.js/node_modules/babel-core/lib/traversal/path/context.js:107:26)

which may be unrelated?

stefanpenner · 2016-06-22T17:45:01Z

commenting out the test above ^, i get further. It seems like i still see

===== 1 JSHint Error

in the console is that expected?

rwjblue · 2016-06-22T17:45:50Z

@stefanpenner - That test has been removed from ember master, because it was fundamentally flawed (and an update to babel-plugin-feature-flags broke it).

stefanpenner · 2016-06-22T17:46:54Z

@rwjblue thanks

stefanpenner · 2016-06-22T17:47:41Z

@Turbo87 once i actually read your instructions and switched to the correct emberjs-build branch i seem to get:

The Broccoli Plugin: [Creator: ember/version.js] failed with:
Error: ENOENT: no such file or directory, symlink '/Users/stefanpenner/src/ember.js/tmp/creator-cache_path-NJJm6B40.tmp/ember/version.js' -> '/Users/stefanpenner/src/ember.js/tmp/
creator-output_path-vMMfS5sw.tmp/ember/version.js'
    at Error (native)
    at Object.fs.symlinkSync (fs.js:899:18)
    at symlink (/Users/stefanpenner/src/emberjs-build/node_modules/broccoli-file-creator/node_modules/symlink-or-copy/index.js:82:14)
    at symlinkOrCopySync (/Users/stefanpenner/src/emberjs-build/node_modules/broccoli-file-creator/node_modules/symlink-or-copy/index.js:58:5)
    at linkFromCache (/Users/stefanpenner/src/emberjs-build/node_modules/broccoli-file-creator/index.js:43:3)
    at Creator.build (/Users/stefanpenner/src/emberjs-build/node_modules/broccoli-file-creator/index.js:31:3)
    at /Users/stefanpenner/src/emberjs-build/node_modules/broccoli-file-creator/node_modules/broccoli-plugin/read_compat.js:61:34
    at lib$rsvp$$internal$$tryCatch (/Users/stefanpenner/src/emberjs-build/node_modules/broccoli-file-creator/node_modules/rsvp/dist/rsvp.js:493:16)
    at lib$rsvp$$internal$$invokeCallback (/Users/stefanpenner/src/emberjs-build/node_modules/broccoli-file-creator/node_modules/rsvp/dist/rsvp.js:505:17)
    at /Users/stefanpenner/src/emberjs-build/node_modules/broccoli-file-creator/node_modules/rsvp/dist/rsvp.js:1001:13
    at lib$rsvp$asap$$flush (/Users/stefanpenner/src/emberjs-build/node_modules/broccoli-file-creator/node_modules/rsvp/dist/rsvp.js:1198:9)
    at doNTCallback0 (node.js:408:9)
    at process._tickCallback (node.js:337:13)

The broccoli plugin was instantiated at:
    at Creator.Plugin (/Users/stefanpenner/src/emberjs-build/node_modules/broccoli-file-creator/node_modules/broccoli-plugin/index.js:7:31)
    at new Creator (/Users/stefanpenner/src/emberjs-build/node_modules/broccoli-file-creator/index.js:15:10)
    at createEmberVersion (/Users/stefanpenner/src/emberjs-build/lib/create-ember-version.js:8:14)
    at CoreObject.enumeratePackages [as _enumeratePackages] (/Users/stefanpenner/src/emberjs-build/lib/ember-build.js:204:45)
    at CoreObject.generateCompiledSourceTree [as _generateCompiledSourceTree] (/Users/stefanpenner/src/emberjs-build/lib/ember-build.js:95:26)
    at CoreObject.getDistTrees (/Users/stefanpenner/src/emberjs-build/lib/ember-build.js:279:34)
    at module.exports (/Users/stefanpenner/src/ember.js/ember-cli-build.js:97:21)
    at CoreObject.module.exports.Task.extend.setupBroccoliBuilder (/Users/stefanpenner/src/ember.js/node_modules/ember-cli/lib/models/builder.js:54:19)
    at CoreObject.module.exports.Task.extend.init (/Users/stefanpenner/src/ember.js/node_modules/ember-cli/lib/models/builder.js:89:10)
    at CoreObject.superWrapper [as init] (/Users/stefanpenner/src/ember.js/node_modules/core-object/lib/assign-properties.js:32:18)
    at CoreObject.Class (/Users/stefanpenner/src/ember.js/node_modules/core-object/core-object.js:32:33)
    at CoreObject.module.exports.Task.extend.run (/Users/stefanpenner/src/ember.js/node_modules/ember-cli/lib/tasks/serve.js:15:19)
    at /Users/stefanpenner/src/ember.js/node_modules/ember-cli/lib/commands/serve.js:64:24
    at lib$rsvp$$internal$$tryCatch (/Users/stefanpenner/src/ember.js/node_modules/rsvp/dist/rsvp.js:1036:16)
    at lib$rsvp$$internal$$invokeCallback (/Users/stefanpenner/src/ember.js/node_modules/rsvp/dist/rsvp.js:1048:17)
    at /Users/stefanpenner/src/ember.js/node_modules/rsvp/dist/rsvp.js:331:11

thoughts?

Turbo87 · 2016-06-22T19:25:43Z

@stefanpenner hmm that looks unrelated. did you make sure to clean all the caches?

Turbo87 · 2016-06-22T20:30:38Z

@stefanpenner can't reproduce your second issue. I'm getting hit by An unknown feature 'fred'' was encountered too, but that should be solved once emberjs/ember.js#13549 is rebased.

stefanpenner · 2016-07-01T16:26:34Z

As discussed in slack, we will need to adjust how the eslint plugin is loaded, so that npm link of emberjs-build works.

I wasn't able to fully test, (headless tests, appear unhappy on my machine but unrelated to this PR), but the rest seems to work good and as expected.

Turbo87 · 2016-07-04T08:56:13Z

As discussed in slack, we will need to adjust how the eslint plugin is loaded, so that npm link of emberjs-build works.

unfortunately this doesn't seem to be possible currently. the only way to specify plugins right now is through their package name (eslint-plugin-ember-internal or just ember-internal) which will be resolved relative to the eslint installation. if npm link is used for emberjs-build those plugins will not be in a parent path of emberjs-build and so they won't be discovered. the only way that this might work with npm link is if the plugins are installed in emberjs-build too.

this might be off-topic but what was the reason for extracting emberjs-build out of the ember.js repo? I guess if the build pipeline was part of ember.js too this problem wouldn't actually exist.

stefanpenner · 2016-07-11T15:18:25Z

this might be off-topic but what was the reason for extracting emberjs-build out of the ember.js repo? I guess if the build pipeline was part of ember.js too this problem wouldn't actually exist.

I'm not sure the abstractions where quite correct, but this is the current state of things. I do believe there is common sentiment to dramatically improve this.

We should chat sometime this week, if eslint cannot be abstracted like this we may need to work to solve it.

Turbo87 · 2016-07-11T17:44:01Z

We should chat sometime this week, if eslint cannot be abstracted like this we may need to work to solve it.

The issue with that is that ESLint 3 was just released a few days ago which dropped Node 0.x support. My guess is that ESLint 2 won't receive any new features or refactorings in that direction so we would have to either drop Node 0.x support for Ember development or live without npm linking this in. (or fork ESLint, but I don't think that would be a proper long term solution)

stefanpenner · 2016-07-11T17:44:37Z

(or fork ESLint, but I don't think that would be a proper long term solution)

:s, sounds like we need to add this to the agenda

Turbo87 · 2016-07-11T17:51:46Z

I'm wondering if @nzakas would answer if I asked if this was something that he would consider for another v2 release...

for context: we are talking about using require('some-package') in plugins in .eslintrc.js

Turbo87 · 2016-07-11T17:53:07Z

and if we'd really fork for the time being then we'd also have to fork broccoli-lint-eslint... :-/

rwjblue · 2016-07-11T23:59:55Z

FWIW, I think we can require a specific node version to build Ember itself...

stefanpenner · 2016-07-12T01:32:23Z

FWIW, I think we can require a specific node version to build Ember itself...

I don't believe so, as we would like people to be able to build ember like they do ember-data. That being said, we could disable eslint in those cases.

rwjblue · 2016-07-12T01:41:41Z

Agreed. Consumers of addons typically do not run linting or tests for those addons.

Turbo87 · 2016-08-09T09:18:00Z

@stefanpenner it seems that eslint/eslint#6865 might solve our issue

Turbo87 · 2016-12-08T17:49:20Z

@stefanpenner @rwjblue this PR is up-to-date again and IMHO ready to be merged.

instructions to try it out:

check out the branch from ESLint ember.js#13549
npm link emberjs-build
ember test --server
produce a lint issue in the ember.js repo (e.g. remove a @public tag from the docs)

Turbo87 · 2016-12-12T08:36:24Z

@stefanpenner FYI I worked around the plugin resolution issue by just not using any plugins. It seems that having custom rules in the repository is also well supported by ESLint and since we can give absolute paths to the folder with the custom rules there is no issue with npm link either.

rwjblue · 2017-01-03T14:26:24Z

lib/ember-build.js

        glimmer: this.glimmer,
-        vendoredPackages: vendoredPackages
+        vendoredPackages: vendoredPackages,
+        eslintRulePaths: ['lib/eslint-rules'],


This should be passed in from ember-cli-build.js in Ember itself.

rwjblue · 2017-01-03T14:33:24Z

package.json

-    "broccoli-jscs": "^1.4.1",
-    "broccoli-jshint": "^1.1.0",
    "broccoli-kitchen-sink-helpers": "^0.2.6",
+    "broccoli-lint-eslint": "^2.3.0",


yeah, that makes more sense now 👍

rwjblue · 2017-01-03T14:36:18Z

A few small changes here, and I think this can land...

Turbo87 · 2017-01-03T17:51:02Z

@rwjblue done

rwjblue · 2017-01-03T18:56:35Z

Looks like Travis is failing due to 0.12 / 0.10 now, I submitted #185 to remove those and will re-kick off the build here (not sure if it will fix it without a rebase though).

Turbo87 · 2017-01-03T21:00:13Z

@rwjblue rebased, thanks!

rwjblue · 2017-01-03T21:18:12Z

Thank you!

rwjblue · 2017-01-03T21:19:51Z

Released as 0.19.0

Turbo87 changed the title ~~Replace JSHint/JSCS with ESLint~~ [WIP] Replace JSHint/JSCS with ESLint Jun 5, 2016

Turbo87 reviewed Jun 9, 2016
View reviewed changes

Turbo87 force-pushed the eslint branch from ad6bddf to 364a2eb Compare June 9, 2016 11:02

Turbo87 changed the title ~~[WIP] Replace JSHint/JSCS with ESLint~~ Replace JSHint/JSCS with ESLint Jun 9, 2016

Turbo87 force-pushed the eslint branch from 364a2eb to 3fbeca3 Compare June 9, 2016 14:53

Turbo87 mentioned this pull request Jun 15, 2016

spring cleaning emberjs/ember.js#13658

Merged

Turbo87 force-pushed the eslint branch from 3fbeca3 to f09ef9e Compare June 21, 2016 12:31

Turbo87 force-pushed the eslint branch from f09ef9e to 41b8258 Compare June 28, 2016 12:04

Turbo87 mentioned this pull request Aug 9, 2016

Fix: Use Node's resolution algorithm for plugins & parsers (refs #3458) eslint/eslint#6865

Closed

Turbo87 force-pushed the eslint branch 2 times, most recently from e662076 to 6e623f1 Compare December 8, 2016 17:22

locks requested review from rwjblue and stefanpenner January 3, 2017 11:16

rwjblue reviewed Jan 3, 2017

View reviewed changes

Turbo87 added 6 commits January 3, 2017 21:59

Add "broccoli-lint-eslint" dependency

1341694

Add ESLintTree and DISABLE_ESLINT envvar

c121fcd

Remove JSHint and JSCS support

7606908

ESLint: Set "rulePaths" option

961cb2d

ESLint: Accept "eslintRulePaths" from outside

ad83c23

Update "broccoli-lint-eslint" to v3.2.0

674a4df

Turbo87 force-pushed the eslint branch from 828bcfe to 674a4df Compare January 3, 2017 20:59

rwjblue merged commit d35e3bd into emberjs:master Jan 3, 2017

Turbo87 deleted the eslint branch January 4, 2017 07:29

Uh oh!

Replace JSHint/JSCS with ESLint #163

Replace JSHint/JSCS with ESLint #163

Uh oh!

Conversation

Turbo87 commented Jun 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Turbo87 commented Jun 5, 2016

Uh oh!

stefanpenner commented Jun 6, 2016

Uh oh!

Turbo87 Jun 9, 2016

Choose a reason for hiding this comment

Uh oh!

Turbo87 commented Jun 9, 2016

Uh oh!

Turbo87 commented Jun 10, 2016

Uh oh!

stefanpenner commented Jun 10, 2016

Uh oh!

Turbo87 commented Jun 18, 2016

Uh oh!

Turbo87 commented Jun 21, 2016

Uh oh!

stefanpenner commented Jun 22, 2016

Uh oh!

stefanpenner commented Jun 22, 2016

Uh oh!

stefanpenner commented Jun 22, 2016

Uh oh!

rwjblue commented Jun 22, 2016

Uh oh!

stefanpenner commented Jun 22, 2016

Uh oh!

stefanpenner commented Jun 22, 2016

Uh oh!

Turbo87 commented Jun 22, 2016

Uh oh!

Turbo87 commented Jun 22, 2016

Uh oh!

stefanpenner commented Jul 1, 2016

Uh oh!

Turbo87 commented Jul 4, 2016

Uh oh!

stefanpenner commented Jul 11, 2016

Uh oh!

Turbo87 commented Jul 11, 2016

Uh oh!

stefanpenner commented Jul 11, 2016

Uh oh!

Turbo87 commented Jul 11, 2016

Uh oh!

Turbo87 commented Jul 11, 2016

Uh oh!

rwjblue commented Jul 11, 2016

Uh oh!

stefanpenner commented Jul 12, 2016

Uh oh!

rwjblue commented Jul 12, 2016

Uh oh!

Turbo87 commented Aug 9, 2016

Uh oh!

Turbo87 commented Dec 8, 2016

Uh oh!

Turbo87 commented Dec 12, 2016

Uh oh!

rwjblue Jan 3, 2017

Choose a reason for hiding this comment

Uh oh!

rwjblue Jan 3, 2017

Choose a reason for hiding this comment

Uh oh!

Turbo87 Jan 3, 2017

Choose a reason for hiding this comment

Uh oh!

rwjblue commented Jan 3, 2017

Uh oh!

Turbo87 commented Jan 3, 2017

Uh oh!

Turbo87 commented Jun 5, 2016 •

edited

Loading