Setup eslint

[benmccann]

You can run with npm run lint. It will also be run automatically on the CI. For now, I only set it up for src (not test)

I copied the config from svelte. We may be able to setup a shared config and/or make the config stricter at some point, but this seemed like a good starting point

The errors that were hardest to fix are in the second commit on this PR. The fixes in the first PR were done automatically by eslint so shouldn't need much review. A lot of the remaining ones I ended up disabling because we had either done things purposefully or because the issue was in another library (e.g. the ignores on import statements were mostly about those libraries not having a default export)

I had to upgrade sucrase because otherwise it would fail on import type syntax

[Conduitry]

I think we want to instead wait for @antony's shared ESLint config to get settled first, and then use that here as well.

[benmccann]

I just tried changing this to use https://github.com/sveltejs/eslint-config. It ended up requiring a handful of stuff to be changed some of which seemed clearly wrong to me. I'm not sure why that is because I thought it was supposed to be exactly the same as the existing config

What do you guys think about merging this as is and then we can work on debugging what the issues are in https://github.com/sveltejs/eslint-config before applying it here?

[antony]

It will do. I took svelte as a baseline, upgraded eslint dependencies, and relaxed the rules which broke to result in 0 code changes. Sapper will be using a slightly different combination of versions and lint config, so you will find errors. Therefore I propose we merge this, tighten rules to where we want to be and get warnings to zero, and then start applying the rules to other projects and fixing them as we go. We should probably look at PRs on those projects before we do though, as tightening lint will make a few of them hard to merge.

On Sat, 6 Jun 2020 at 20:02, Ben McCann ***@***.***> wrote: I just tried changing this to use https://github.com/sveltejs/eslint-config. It ended up requiring a handful of stuff to be changed some of which seemed clearly wrong to me. I'm not sure why that is because I thought it was supposed to be exactly the same as the existing config What do you guys think about merging this as is and then we can work on debugging what the issues are in https://github.com/sveltejs/eslint-config before applying it here? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1198 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABVOROLQZLYJPXZ6OETDD3RVKHETANCNFSM4NBYS4QA> .

--

…

________________________________ ꜽ . antony jones . http://www.enzy.org

[antony]

@benmccann my opinion is that this PR should be closed. It does something pointless because it applies a now out-of-date lint configuration, and makes code changes, which will mean that other open PRs can no longer be merged cleanly.

The other part of your suggestion is the path I think we should take - fix the issues with linting on the svelte repository (attempting to fix as much as possible, without drastic code changes), and then apply it here, again without impacting open PRs too much.

Thoughts?

[benmccann]

It does something pointless because it applies a now out-of-date lint configuration, and makes code changes, which will mean that other open PRs can no longer be merged cleanly.

The common configuration package applies these changes plus others, so you will still need these changes either way

apply it here, again without impacting open PRs too much.

I'm not sure how you can control how much open PRs are impacted. The impact to most PRs will be minimal though

Anyway, this doesn't lint cleanly anymore since rebasing against master. There are three new issues from implementing the export queue. I'm not going to be able to find the time to fix those. I think that taking this PR, fixing those three issues, and then merging might be a helpful intermediate waypoint in getting towards the eventual goal of using the common Svelte config, which applies these changes and then more, but if you want to wait until the common config is implemented that's reasonable too. Feel free to do whatever you want with this PR whether that's fixing it up, closing it, etc.

[antony]

I'm not sure how you can control how much open PRs are impacted. The impact to most PRs will be minimal though

I don't think we can, we just have to manage it as best as possible.

I'll take this PR and see if I can turn the rules into overrides to avoid any code changes, with a view to trying to remove the overrides to leave a clean configuration, as we close some PRs.

Thanks!

[benmccann]

I'll take this PR and see if I can turn the rules into overrides to avoid any code changes

I'm not sure if it's possible because the code is in an inconsistent state. E.g. one of the biggest changes was how do you separate fields in a type - do you use ; or ,? Right now it's half and half. You'd have to turn off the eslint rule to leave the code in its current state.

The PRs that will have conflicts will mostly just be a couple characters on a couple lines. If you wait for the backlog of PRs to get merged first you might be waiting almost indefinitely. Most of the ones remaining are waiting for configuration to be figured out or are not in a mergable state

Anyway, just my 2 cents. Thanks for taking this over!

[antony]

@benmccann the way I did it for svelte is to set the rules how we want them and turn them to warn not error. There's a lot of warnings on svelte right now but it's a good first call to cleanup.

I'll see what I can do, anyway :)

[thgh]

Just my 2 cents: by using lint-staged you can run eslint only on staged files. Combine that with husky and every commit will automatically fix itself. This allows you to upgrade the codebase gradually without introducing too many conflicts and requires no effort from contributors.
(cherry on top would be using prettier)

[antony]

As an update on this, I've started working to update Svelte to follow its own linting rules. There is some mutation of node built-ins (in tests) going on which are also dependent on run-order, so it will take a bit of untangling of that first. I'll open a draft PR when I'm able.