-
Notifications
You must be signed in to change notification settings - Fork 5
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
Comments
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? |
Yes exactly, that's my idea. Transient dependencies don't matter as you can copy whole node_modules to vendor/node_modules |
Now that 2.0.0 is out and all extensions are updated, I'll work on this next. |
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:
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 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. |
I can only vendor eslint-config-, but the eslint-plugin- you would still need to install. |
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 The only remaining problem to solve is how to force eslint to resolve plugins which are listed in configs
and the same for all other fields that plugin can export |
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 |
Because you don't want peerDependencies warnings |
Got it working, didn't need to vendor. Here's the config: https://github.com/KidkArolis/eslint-config-healthier/blob/master/eslintrc.json And here's the output: With just installing
The only caveat, which I'm not sure if it's a big deal, is that the custom rules are all named Give it a go and let me know if it's working for you:
|
It's bummer that rules are prefixed because I override rules without them.. Now I need to do something like:
So half or rules have prefix, half not... Why such decision? Could you remove this prefix? |
Also I think prettier's configs won't disable the rules you've decided to prefix |
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 |
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".
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? |
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. |
I don't want to turn it off, I want to turn it on. |
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 |
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 |
Phew, should have checked your latest comments first, just tried and the rule does work: 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: But the good news is – this config works! With the |
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 But! We could do one better and bundle this is an extra config! So you would do:
I'll work on that. |
Done, so now:
|
You think this is ready for 1.0.0 release? |
I believe that healthier should have good defaults, and forcing to use custom config with Maybe you could consider enabling react rules by default or discovering whether |
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. |
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. |
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. |
If you decide to discover dependencies please do it relative to 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 |
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 I'm closing this and will continue the discussion in #20. |
Published as 1.0.0 for now. |
Also, just haven't had time to work in the auto detection. It would have to be done both for |
Hello,
I'd gladly use
extends: ["healther"]
in my .eslintrc. Could you publish actual package with eslint configuration for this package?The text was updated successfully, but these errors were encountered: