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

Let's factor out eslint configs into it's own sharable eslint config #636

Closed
Gozala opened this issue Sep 10, 2020 · 7 comments
Closed

Let's factor out eslint configs into it's own sharable eslint config #636

Gozala opened this issue Sep 10, 2020 · 7 comments
Assignees

Comments

@Gozala
Copy link
Contributor

Gozala commented Sep 10, 2020

Problem

All the repos that use aegir for linting confuse editors eslint plugins, because they are unaware of the eslint configs bundled with aegir. My personal experience had been that vscode would reformat things on some base eslint assumbtions on save that would later fail on CI. There is a workaround for vscode via .vscode/settings.json, but it is not documented, only works for vscode and generally harms an onboarding first contribution experience.

Proposal

I would like to propose:

  1. To factor out configs used by aegir and create an ESLint shareable config
  2. And add .eslintrc files in our repos with content { extends: "ipfs" }

That would enable configuration sharing, and address the described problem. I already have a proof of concept for this and I have tested it on js-ipfs repo to ensure that it works as expected.

@hugomrdias could you please provide your feedback as you probably have a context in regards to why current setup was chosen over sharable configs (if they existed back then).

@Gozala Gozala self-assigned this Sep 10, 2020
@hugomrdias
Copy link
Member

The purpose of aegir is to reduce tooling burden on repos to almost 0. A shared config would require prs to all repos plus the burden to keep it updated, etc.

We had a shared config previously but it was abandoned because it was always out dated and caused problems in the aegir lint command because of multiple versions in the dep tree.

The current setup is easier and stable to maintain and users can just use the lint cmd or do a one time editor setup and never think about it again.

We should definitely document the editor config and the aegir lint in the contribute guidelines but besides that i see more cons than pros. Let's see what others think.

@Gozala
Copy link
Contributor Author

Gozala commented Sep 10, 2020

ok how about we make that shared config aiger dependency so that it does not need to be installed manually ? I still think it would be worth adding .eslintrc file even if it points to config in aegir so things work out of the box & does not require setting up tooling to make it work.

@hugomrdias
Copy link
Member

do we need the shared config package ? if we add the .eslintrc ?

I would rather use the package.json eslint property i hate extra files 😝

@vmx
Copy link
Member

vmx commented Sep 10, 2020

BTW: it seems like @mkg20001 is having a similar setup: https://github.com/mkg20001/eslint-config-aegir-standalone so perhaps he can tell how he is using it in his dev environment.

@mkg20001
Copy link
Contributor

I have a script for generating the standalone package from aegir itself https://github.com/mkg20001/generate-aegir-standalone, that package gets then included in my .eslintrc via extends option

Additionally I have a template in mkgs-tool (thing I made for managing/synchronising dotfiles accross repos) that I'm using for my js projects to include aegir-standalone everywhere https://github.com/mkg20001/mkgs-tool/blob/master/src/templates/eslint.js#L8

@lidel
Copy link
Member

lidel commented Sep 14, 2020

The main benefit of having .eslintrc (or eslintConfig field in a package.json) loading a shared config is that it is generic and does not require contributors to learn about aegir and apply custom configuration for their editor.

do we need the shared config package ?

If we have it, then it can also be used in end-user projects which do not use aegir but want to follow the same code conventions (examples: ipfs-desktop, ipfs-companion, ipfs-webui..)

@hugomrdias would below address your concerns? (introduces no new files, no direct deps 👌)

  1. Make shared config package a dependency of aegir, so it does not need to be explicitly listed as a dependency of each project
  2. Make aegir lint start with validation of .eslintrc /eslintConfig field in a package.json with extends: "ipfs", and fail lint if missing, printing how to fix:
    Make sure eslintConfig field in a package.json includes blah blah – <link to docs>
    
    This way we won't have to PR all the projects at once, switch will happen over time, as a part of regular dependency bumps performed by project maintainers

@Gozala
Copy link
Contributor Author

Gozala commented Sep 15, 2020

Fixed by #638

@Gozala Gozala closed this as completed Sep 15, 2020
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

No branches or pull requests

5 participants