digitalbazaar / eslint-config-digitalbazaar

BSD 3-Clause "New" or "Revised" License
2 stars 1 forks source link

make each eslint rule stand alone #21

Closed aljones15 closed 5 years ago

aljones15 commented 5 years ago

this comes from the StatsService PR:

EDIT: @dlongley added a link to the direct comment by @gannan08: https://github.com/digitalbazaar/bedrock-web-stats/pull/3#discussion_r266011848

Basically the idea is this

eslint-config-digitalbazaar - this is a standalone base set of rules eslint-config-digitalbazaar/vue - this is only the vue rules eslint-configdigitalbazaar/jsdoc - this is only the jsdoc rules

right now vue and jsdoc extend the base rules ensuring that regardless of what rule set you use the base rules are enforced.

here after we will just use the shortcuts to refer to them.

EDIT: @dlongley added a link to the direct comment by @mattcollier: https://github.com/digitalbazaar/bedrock-web-stats/pull/3#discussion_r266042690

This actually won't effect as many repos as you might thing as the root .eslintrc.js is often set to digitalbazaar with digitalbazaar/vue being in the components dir.

This will require refactoring some repos namely standalone bedrock-vue repos will need to be refactored to use ['digitalbazaar', 'digitalbazaar/vue']

in terms of this project we just have to drop:

extends: ['index.js']

from vue and jsdoc and update the README.

Projects that might need to be updated.

dlongley commented 5 years ago

I'm trying to figure out what's being asked of me or what the specific problem is -- it just sounds like there is "some linting issue" or "this doesn't work the way I thought it should work". But that results in what happening? Are we following standard practice? What's the real issue, are we having conflicts between rules and some new pattern will resolve it? What's going on?

Can this be presented as two different, specific proposals so I can say which one I support? Sorry for having a hard time digesting this. Please make it clear what the options are like this:

Problem: The problem is "foo". Here are the ways we can fix it:

Proposal 1: "We do A and this will result in B." Proposal 2: "We do X and this will result in Y."

aljones15 commented 5 years ago

@dlongley just looking for consensus. I think this best summarizes it: Do we want to make our eslint rules composable or stick with the current all rules extend the base rules.

We have 3 rule sets: 1.eslint-config-digitalbazaar aka the base rules.

  1. vue (extends base rules)
  2. jsdoc (extends extends base rules)

Proposal One: All rules extend the base rules to ensure base rules are always there. Proposal Two: vue and jsdoc are separate rule sets that do not extend the base rules.

Proposal One Problems: Proposal One is heavy if I decide in my project to do this:

extends: [digitalbazaar, digitalbazaar/vue, digitalbazaar/jsdoc]

I am extending the base rules, extend vue which also extends the base rules, and jsdoc which also extends the base rules. So basically I have 2 duplicate extensions of the base rules.

Additionally let's say on my project I do not want the base rules, but I do want the jsdoc rules. Then I have no choice because jsdoc extends the base rules by default. It's not possible to drop the base rules.

Proposal Two Problems: with Proposal Two the base rules are only there if I explicitly extend them:

extends: [digitalbazaar, digitalbazaar/vue] // base rules in linter
extends: [digitalbazaar/vue] // only vue rules in linter

So all I really need is consensus on which pattern. I started this project with proposal one because I was thinking we wanted to guarantee the base rules are there.

dlongley commented 5 years ago

So it sounds like proposal two is more flexible where there are actually use cases where we need that flexibility? We just need to make sure we don't leave something out in the eslint config? If that's what you're saying, let's do proposal 2.

cc: @mattcollier, @davidlehn, @gannan08 please weigh in.

mattcollier commented 5 years ago

I like proposal 2 as well.

It is my expectation that if one has extends: ['digitalbazaar'] in the root of a project, and extends: ['digitalbazaar/vue'] in the components folder, that eslint is summing up the rules so that the base rules and the vue rules are in effect in thecomponents` folder.

I believe this behavior is controlled by the root property in the base config: https://eslint.org/docs/user-guide/configuring#using-configuration-files-1

aljones15 commented 5 years ago

I actually like proposal 2 more too so I will take out the extends later today. while I'm at it can we add the eslint import plugins? https://github.com/benmosher/eslint-plugin-import

Consensus on that. it just be in base rules.

Proposal: add the eslint-plugin-import so we can make sure all imports resolve to real files.

mattcollier commented 5 years ago

@aljones15 is the eslint-plugin-import going to address this issue? https://github.com/digitalbazaar/eslint-config-digitalbazaar/issues/20 If so, I'm +1 to getting that fixed somehow.

aljones15 commented 5 years ago

@aljones15 is the eslint-plugin-import going to address this issue? #20 If so, I'm +1 to getting that fixed somehow.

don't believe so looks like that can be resolved per project by using babel-eslint: https://github.com/babel/babel-eslint i.e. the rules we have are just fine it's just the linter you are using.

yeah question was asked to eslint again recently and https://github.com/eslint/eslint/issues/9927 and they still recommend babel-eslint.

mattcollier commented 5 years ago

OK, then what is that other eslint-plugin-import going to do for us?

aljones15 commented 5 years ago

OK, then what is that other eslint-plugin-import going to do for us?

bold means I want it italic means let's discuss

the major issue here really is browser vs node.

mattcollier commented 5 years ago

I think that additional module needs a separate PR and some testing experience.

aljones15 commented 5 years ago

I think that additional module needs a separate PR and some testing experience.

yeah I agree my bad putting in this issue. removing the imports in a moment.