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

Publish eslint-config-healtiher #16

Closed
sheerun opened this issue Jan 30, 2019 · 29 comments
Closed

Publish eslint-config-healtiher #16

sheerun opened this issue Jan 30, 2019 · 29 comments
Labels
enhancement New feature or request

Comments

@sheerun
Copy link

sheerun commented Jan 30, 2019

Hello,

I'd gladly use extends: ["healther"] in my .eslintrc. Could you publish actual package with eslint configuration for this package?

@KidkArolis
Copy link
Owner

Will do, hopefull this week. If you'd like to contribute code in any way, let me know how I could unblock that.

You said before that I should vendor in all of the dependencies (somehow? what if they have transient dependencies) and require all of the rules relatively so that the upstream consumers of eslint-config- healthier don't need to install any dependencies, right?

@KidkArolis KidkArolis added the enhancement New feature or request label Jan 30, 2019
@sheerun
Copy link
Author

sheerun commented Jan 30, 2019

Yes exactly, that's my idea. Transient dependencies don't matter as you can copy whole node_modules to vendor/node_modules

@KidkArolis
Copy link
Owner

Now that 2.0.0 is out and all extensions are updated, I'll work on this next.

@KidkArolis
Copy link
Owner

I've looked a little bit into this, and I'm not sure this is really possible without some hackery. Specifically, it's not possible to abstract the 3 configs and 5 plugins used by healthier into 1 eslint thing.

I had a brief look at eslint/eslint#3458 and eslint/rfcs#5 and it's quite a conversation 😅

I don't think vendoring will help, because eslint-config-standard and eslint-config-standard-jsx depends on plugins. Plugins are specified as simple strings: ["import", "node", "promise", "standard"] and so eslint requires them in from the root of your project, if they're not there, it's not gonna work. I don't think I can redirect eslint to require these plugins from my vendor dir. I tried pointing to a plugin using a relative path, but got this error:

 Cannot find module 'eslint-plugin-./vendor/node_modules/eslint-plugin-import'

If this was doable, I think eslint repo wouldn't have so much conversation on the topic.

Have you done something like this before?

So, I can still publish eslint-config-healthier. But it's gonna basically require you to install 5 plugins as well. Just like when you use eslint-config-standard. I can do still create this package, just not sure if it's that helpful? It's a bit easier to use in that you just need to remember to install 1 thing and then eslint will tell you which plugins are required.

Another hacky thing is to declare those plugins as dependencies and then hope that npm/yarn hoists them to the top level and eslint will grab them from there, if you're lucky, but this might not always work.

@KidkArolis
Copy link
Owner

I can only vendor eslint-config-, but the eslint-plugin- you would still need to install.

@sheerun
Copy link
Author

sheerun commented Feb 1, 2019

The peerDepenencies warning part of eslint/eslint#3458 can be solved by vendoring plugins. There won't be no warning because npm doesn't actually install them.

It still seems eslint won't resolve these plugins unless they are at the top level, until eslint/rfcs#7 is implemented, which has been accepted day ago.

Regardless, I think it's safe to assume that if you create eslint-plugin-healthier and mark it as dependency of eslint-config-healthier, it'll be installed at top level becase most of modern package managers (recent npm and yarn), use dependency flattening. After eslint/rfcs#7 is implemented this solution will work even for legacy npm versions.

The only remaining problem to solve is how to force eslint to resolve plugins which are listed in configs eslint-config-healthier depends on. I think the best way is to:

  1. Vendor all plugins inside eslint-plugin-healther and all configs inside eslint-config-healther
  2. Change vendoring script for configs so it also removes all "plugins" references from any eslint.json found and replaces them with "plugins": ["healthier"]
  3. Let eslint-plugin-healther act as meta-plugin for all plugins it vendors, something like:
const plugins = ['standard', 'react']

module.exports = {
  rules: plugins.reduce((rules, name) => Object.assign(rules, require('./vendor/node_modules/eslint-plugin-' + name).rules), {})
}

and the same for all other fields that plugin can export

@KidkArolis
Copy link
Owner

Yeah, this could work.

One question though. If we're relying on npm to install eslint-plugin-healthier to top level anyway, why not just rely on npm to install eslint-plugin-standard, eslint-plugin-node, etc. into top level, which means this should work without having to vendor plugins.

In other words, if you install and extend just eslint-config-healthier, but it declared all of it's dependencies, they will probably be hoisted to the top and so eslint will actally find them?

@sheerun
Copy link
Author

sheerun commented Feb 1, 2019

Because you don't want peerDependencies warnings

@KidkArolis
Copy link
Owner

Got it working, didn't need to vendor.

Here's the config: https://github.com/KidkArolis/eslint-config-healthier/blob/master/eslintrc.json
And the plugin: https://github.com/KidkArolis/eslint-plugin-healthier/blob/master/index.js

And here's the output:

image

With just installing eslint-config-healthier (and even if I move all configs into eslint-config-healthier/node_modules and all plugins into eslint-plugin-healthier/node_modules), we get:

  • standard checks working
  • plugin based checks working (e.g. node deprecation, promise naming)
  • no styling rules applied due to prettier config (e.g. mixing semis)

The only caveat, which I'm not sure if it's a big deal, is that the custom rules are all named healthier/X now, but oh wells.

Give it a go and let me know if it's working for you:

npm i -D KidkArolis/eslint-config-healthier

@sheerun
Copy link
Author

sheerun commented Feb 2, 2019

It's bummer that rules are prefixed because I override rules without them..

Now I need to do something like:

  "rules": {
    "healthier/react/prop-types": "off",
    "no-eval": "off"
  },

So half or rules have prefix, half not... Why such decision? Could you remove this prefix?

@sheerun
Copy link
Author

sheerun commented Feb 2, 2019

Also I think prettier's configs won't disable the rules you've decided to prefix

@sheerun
Copy link
Author

sheerun commented Feb 2, 2019

Also some rules are not working at all, e.g. "react/prop-types"

I think it's because you've prefixed them but they are declared without prefix in plugins

@KidkArolis
Copy link
Owner

I'll have a closer look, but it's not that I "decided" to prefix. The way eslint works is that when it sees a rule, e.g. "react/prop-types" it gets the plugin by that name, i.e. require('eslint-plugin-react'). Instead, I rename (not prefix) all custom rules to "healthier/prop-types" and now eslint does require("eslint-plugin-healthier") and finds the rule "prop-types" there.

So eslint's rules are not prefixed (as usual), e.g. "no-eval".
And all plugin based rules become "healthier/rule-name", e.g.:

react/prop-types        -> healthier/prop-types
node/no-deprecated-api  -> healthier/no-deprecated-api
promise/param-names     -> healthier/param-names

So it's not "react/prop-types" or "healthier/react/prop-types", it's "healthier/prop-types", and you need to turn this one on, because standard (and so healthier) doesn't turn that on by default.

The reason prettier config should be working is because I also rename rules in all relevant configs: https://github.com/KidkArolis/eslint-config-healthier/blob/master/configs/convert.js#L10-L12.

The renaming has to happen to avoid eslint trying to require all those plugins. At least until they fix elint..

Do you have a specific piece of code + specific eslintrc where something's not working as expected?

@KidkArolis
Copy link
Owner

Btw, by the same logic, we could instead directly depend on all those plugins and they would also be hoisted by npm. And then we wouldn't need any renaming of rules. And there is no peerDeps issues, because only shared configs peer depend on plugins. I don't think plugins depend on anything.

But in this way, it's easier to install both eslint-config-healthier and eslint-plugin-healthier, if you want to be "nice" and not rely on hoisting.

@sheerun
Copy link
Author

sheerun commented Feb 4, 2019

I don't want to turn it off, I want to turn it on. healthier/prop-types doesn't work

@sheerun
Copy link
Author

sheerun commented Feb 4, 2019

Or maybe little differently: I'd like it to be enabled by default when I use healthier config.

I suspect it's because you don't re-export configs: https://github.com/yannickcr/eslint-plugin-react/blob/master/index.js#L117

@sheerun
Copy link
Author

sheerun commented Feb 4, 2019

Hmm, now I think it's because you don't extend eslint-config-react which enables rules in eslint-plugin-react: https://github.com/KidkArolis/eslint-config-healthier/blob/master/eslintrc.json#L1

@KidkArolis
Copy link
Owner

Phew, should have checked your latest comments first, just tried and the rule does work:

image

The reason I didn't turn it on by default is because Standard doesn't turn it on by default. Healthier is not currently deviating from Standard and I'm not sure it should, see:

image

But the good news is – this config works! With the healthier/ caveat.

@KidkArolis
Copy link
Owner

I think the reason Standard doesn't lint react by default, is because, well, not everyone is using react and this would break it for them. The nice thing about healthier is that unlike standard, it's extensible via .eslintrc, so you can extend: ['healthier', 'standard-react']:

image

But! We could do one better and bundle this is an extra config! So you would do:

extends: ["healthier", "healthier/react"]

I'll work on that.

@KidkArolis
Copy link
Owner

Done, so now:

  • by default healthier behaves like standard (at least for now)
  • you can still turn on/off manually any rule from node/promise/imports/react/jsx with "healthier/" prefix
  • you can do "extends": ["healthier", "healthier/react"] to opt into standard's react rules (not sure they're well maintained, but it's a good set of rules)

@KidkArolis
Copy link
Owner

You think this is ready for 1.0.0 release?

@sheerun
Copy link
Author

sheerun commented Feb 4, 2019

I believe that healthier should have good defaults, and forcing to use custom config with "extends": ["healthier", "healthier/react"] for react projects isn't good one I think.

Maybe you could consider enabling react rules by default or discovering whether react is dependency of project and enabling it automatically?

@KidkArolis
Copy link
Owner

Are you sure? Not all projects are react, e.g. and especially in node projects, this wouldn't be an issue.

Detecting dependency, could be done maybe, looks like that's how eslint-config-react extracts react version already, I think. This is easy enough to do I guess. Not gonna always work, say when using preact (could detect that too). I wonder why standard didn't do it that way. Maybe cause it wouldn't be consistent if you just ran standard against some random file on your machine.

@sheerun
Copy link
Author

sheerun commented Feb 4, 2019

There are multiple packages that you could detect e.g. react, preact, react-dom, react-scripts (used by create-react-app) etc. I think the best part about prettier is that you don't need to configure it mostly, e.g. the same could be said about scss or vue, not everyone uses it, but prettier can format it.

@sheerun
Copy link
Author

sheerun commented Feb 4, 2019

Also vue could enable vue plugin and configuration, jest could enable jest plugin and configuraton. There are multiple possibilities: https://github.com/dustinspecker/awesome-eslint#frameworks-and-libraries

Configs and plugins are usually really lightweight so I wodun't worry that you list many of them as dependencies. In the worst case you can vendor all dependencies like I do for bower, so installation is lightning fast regardless of how many dependencies you have.

@sheerun
Copy link
Author

sheerun commented Feb 4, 2019

If you decide to discover dependencies please do it relative to process.cwd() instead of __dirname because healthier could be installed globally.

Of course it would be ideal that healthier used eslint-config-healthier directly after you publish 1.0.0 instead of duplicating effort to gather these configs and plugins

@KidkArolis
Copy link
Owner

Released eslint-config-healthier and eslint-plugin-healthier as 1.0.0 to not hold that up.

Can release 2.0.0 once we add React detection. For now, it can already be used. I'm already using healthier itself in like 3 repos.

I'm closing this and will continue the discussion in #20.

@KidkArolis
Copy link
Owner

Published as 1.0.0 for now.

@KidkArolis
Copy link
Owner

Also, just haven't had time to work in the auto detection. It would have to be done both for healthier and eslint-config-healthier. And I'm still a bit unsure about the healthier/ rule hijacking, so to me eslint-config-healthier is a bit more experimental. I wonder when eslint will release the updated plugin api.

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

No branches or pull requests

2 participants