Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move eslint-config-ember to dependencies #43

Conversation

alexlafroscia
Copy link
Collaborator

When listed as a devDependency, I noticed that the application that had ember-cli-eslint installed also needed eslint-config-ember within its own dependencies. By moving it to be a dependency of the addon instead, that could be avoided, which makes this addon a little easier to manage, since you don't have to adjust the version of eslint-config-ember directly.

@BrianSipple
Copy link
Collaborator

LGTM. @jonathanKingston, is there anything we might be missing that might cause trouble with this approach instead of the original?

@alexlafroscia
Copy link
Collaborator Author

A few things that I thought of:

  • I'm pretty sure that this only works because of the fact that NPM version 3 pulls dependencies "upward" as much as they can, so the ember-cli-eslint dependency ends up in the root project's node_modules directory because of that. Not sure if this works on older versions of NPM, but also not sure if that matters.
  • It might be advantageous for a project to explicitly define the eslint-config-ember version directly; this should still work after this change, and it should grab an explicitly-defined version if it's there. This just removes the requirement of having it there if you want to use the default.

@jonathanKingston
Copy link
Member

The reason this is a devDependency is so ember-cli-eslint when testing can use that config itself, I think you are conflating both the use-cases here.

I'm not convinced we want to do this, the initial thinking was that the user would install ember-cli-eslint and they would get a suggested setup not that it was required that they must use it. The plan was to let the author manage that themselves but install it and set it up so they had a working solution with zero setup.

As mentioned this won't work in older npm's either.

@Turbo87
Copy link
Member

Turbo87 commented Apr 8, 2016

I agree with @jonathanKingston, I don't want eslint-config-ember in the current state in my project due to the nightmare mode rules.

@mmun
Copy link
Member

mmun commented Apr 12, 2016

@jonathanKingston Can we make eslint-config-ember opt-in instead of including it in the blueprint? And make the default .eslintrc's as vanilla as possible? I want to manage my own .eslintrc and its weird that I have to uninstall an unrelated package immediately after installing this one.

@Turbo87
Copy link
Member

Turbo87 commented Apr 12, 2016

I think I'd prefer that too. Extending from eslint:recommended might be fine though, which doesn't need to install an extra package.

@mmun
Copy link
Member

mmun commented Apr 12, 2016

That's exactly what I have in my current projects that use ember-cli-qunit-eslint, though I'd love to rally the community around this one instead! FWIW here's what I was using:

// .eslintrc
{
  "parser": "babel-eslint",
  "env": {
    "es6": true,
    "browser": true
  },
  "extends": "eslint:recommended"
}
// tests/.eslintrc
{
  "parser": "babel-eslint",
  "env": {
    "es6": true,
    "browser": true,
    "embertest": true
  },
  "extends": "eslint:recommended"
}

You might be able to simplify this even further. The embertest environment is built into eslint and lets you use the global test helpers like visit, click, etc. without a warning.

@Turbo87
Copy link
Member

Turbo87 commented Apr 12, 2016

I'm using this in two of my projects:

{
  "root": true,
  "parserOptions": {
    "ecmaVersion": 6,
    "sourceType": "module"
  },
  "extends": "eslint:recommended",
  "env": {
    "browser": true,
    "node": false
  },
  "rules": {
    // ...
  }
}

this also removes the need for babel-eslint to be installed because the built-in parser of ESLint also has ES6 support

@alexlafroscia
Copy link
Collaborator Author

I think I like the approach that you all are suggesting better, too:

  1. Remove eslint-config-ember from the default config
  2. Make the blueprint generate a really small, really simple config that does not have any third-party dependencies (other than the recommended on, which I'm assuming is built into the tool)

I've shifted a few projects to ESLint and ended up needing to make really substantial changes to the config in every case anyway, relaxing rules when necessary. It would have been much easier to configure if the whole config was in one file, rather than pulling rules from eslint-config-ember too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants