FLUID-5963: initial implementation #1
Conversation
|
@amb26 could you please review this and publish to NPM after it is merged. |
| }); | ||
|
|
||
| // Load the plugin(s): | ||
| grunt.loadNpmTasks("fluid-grunt-eslint"); |
There was a problem hiding this comment.
This project doesn't contain any .js files and we imagine never well - the eslint tasks directed at the project itself should be removed
There was a problem hiding this comment.
Sorry, ignore this comment - of course there is the Gruntfile.js itself
| all: ["**/*.js"] | ||
| }, | ||
| jsonlint: { | ||
| all: ["*.json", ".*.json"] |
There was a problem hiding this comment.
@amb26 ".*.json" is required to jsonlint the ".eslintrc.json" file. It seems that "*.json" doesn't pick this up, possibly because of the leading "." on the ".eslintrc.json" filename.
| "fluid-eslint": ">=2.0.0" | ||
| }, | ||
| "devDependencies": { | ||
| "grunt": "0.4.5", |
There was a problem hiding this comment.
This should be at the 1.x level as with all our modern projects
| }, | ||
| "homepage": "http://fluidproject.org", | ||
| "peerDependencies": { | ||
| "fluid-eslint": ">=2.0.0" |
There was a problem hiding this comment.
Is this a real requirement? It seems rather restrictive. There's no reason why the config couldn't equally well be used with mainstream versions of eslint. I think this should probably be removed.
There was a problem hiding this comment.
eslint indicates that we should declare a peer dependency on it ( http://eslint.org/docs/developer-guide/shareable-configs#publishing-a-shareable-config ). This will cause NPM to throw a warning if the peer dependency is not installed. However, our projects do not actually depend on eslint, but rather a fork "fluid-eslint" instead.
I agree that this module should work with either, although we haven't tested it with the main line of eslint. However, we still need to declare the peerDependency to ensure that it is present. Here's an article on peerDependencies ( https://nodejs.org/en/blog/npm/peer-dependencies/ ).
I pinged you in the channel last Friday to ask about what the difference between our fork and the mainline are, and if we could just switch back to use the main line of eslint now. ( https://botbot.me/freenode/fluid-work/2016-09-23/?msg=73496446&page=1 ). I think the ideal case will be to switch to use the mainline of eslint and not maintain a separate fork. If that's still not possible though, we'll likely either have to restrict this module to only work with our fork, or to remove the validation that the eslint/fluid-eslint is actually present.
There was a problem hiding this comment.
I think we can remove the validation warning since I don't think it is doing much for us and is probably actively misleading. The npm team have acknowledged that peerDependencies is a broken system and have deprecated it in npm 3. The reason for our fork is that the processing of .eslintignore is broken in the main line edition.
There was a problem hiding this comment.
The reason for our fork is that the processing of .eslintignore is broken in the main line edition.
Is that still true in latest versions, and are we tracking it in any way?
Also i'll remove the peerDependency. Seems if it's deprecated anyways, it shouldn't be an issue to drop it.
There was a problem hiding this comment.
As far as I can tell we can track the issue here: ember-cli/ember-cli-eslint#63 - doesn't look like progress is likely
| @@ -0,0 +1,33 @@ | |||
| { | |||
| "name": "eslint-config-fluid", | |||
| "version": "0.1.0", | |||
There was a problem hiding this comment.
May as well be 1.0.0 - we do expect it to work
|
@amb26 ready for more review. I've left replies to your comments and/or addressed the concerns you raised. |
Contains the initial implementation of a distributable eslint configuration.
https://issues.fluidproject.org/browse/FLUID-5963