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

Introduce ESLint to svelte sources #2958

Merged
merged 12 commits into from
Jun 9, 2019
Merged

Conversation

43081j
Copy link
Contributor

@43081j 43081j commented Jun 5, 2019

Fixes #2951.

Are you interested in this?

Interestingly the PR guidelines/template states to remember to run npm run lint, but there wasn't such a script before..

Here's what I've done:

  • Cleared most of .eslintignore as those files don't seem to exist anymore
  • Added ESLint, the typescript parser/plugin and the import plugin (which your config already tried to extend)
  • Disabled the rules which seem to go against your conventions (e.g. camel case identifiers)
  • Added ESLint as an npm script for test/ and src/

If you're interested in taking this PR on im happy to sort the mass of lint errors currently produced so we can get it merged. Most are just inconsistencies with whitespace or interface separator style (semicolon vs comma).

@run1t
Copy link

run1t commented Jun 5, 2019

I Just open an issue about this yesterday (#2958) 😄
I'd be happy to help if you need to !

@43081j
Copy link
Contributor Author

43081j commented Jun 5, 2019

alright so tonight i fixed the many thousands of lint errors.

some by disabling rules (as svelte's conventions differ), some automatically (--fix) and some myself.

any rules I disabled were either because we had a large enough amount of code that it clearly didn't match eslint's convention or because typescript conflicts with it.


Here's a note to self and anyone who is interested:

  • Odd indentation
    • nodes/interfaces L33
    • render-dom/index L79
    • wrappers/EachBlock L312
    • wrappers/IfBlock L213
    • wrappers/InlineComponent L234
    • wrappers/Window L142
    • compiler/interfaces L10 + L41
  • Odd unicode in some files (dashes usually in comments)

These are some oddities that happened after eslint --fix, the unicode especially is scattered around. I read through the entire diff and these are the places that looked out of place.


Also, I left the lint fixes in their own commit in case we want to discard them and keep the setup/config.

On a side note, I would highly recommend we simply turn off all of eslint's indentation rules and introduce a code formatter such as prettier/clang-format. It is better and more reliable IMO to auto-format the code than try lint it (clearly as you see in this PR it doesn't always get it right).

Copy link
Member

@Rich-Harris Rich-Harris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you, this looks great! i just had a few questions as i was looking at the result, commented inline

src/compiler/compile/render-dom/wrappers/IfBlock.ts Outdated Show resolved Hide resolved
src/compiler/compile/render-dom/wrappers/IfBlock.ts Outdated Show resolved Hide resolved
@@ -14,13 +14,13 @@ export default class TitleWrapper extends Wrapper {
block: Block,
parent: Wrapper,
node: Title,
strip_whitespace: boolean,
next_sibling: Wrapper
_strip_whitespace: boolean,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a rule for controlling whether unused variables need a leading underscore? I'm in two minds about it...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ESLint's rule is configurable such that those matching the specified pattern are allowed.

I originally set it to ^_, but actually later enabled typescript's built in (stricter) unused checks at compile time and turned the eslint rule off.

TypeScript has built in support for ignoring _ prefixed locals/parameters. I wouldn't go with another convention because this is built into typescript its self.

@Rich-Harris
Copy link
Member

One other thing — a lot of the changes are reordering functions etc. Is that necessary? It renders the git history slightly less useful...

@43081j
Copy link
Contributor Author

43081j commented Jun 6, 2019

I do realise that and it comes down to preference really.

We can either turn the rule off and leave it as it was, or turn it on and reorder them.

I noticed its pretty inconsistent, so wasn't sure which would be preferred. Many files have functions at the start, and at the end. Many have them at the end only and some have them at the start only.

I don't mind discarding most of the fixes and redoing them at some point if we choose to leave the order intact.

@Conduitry
Copy link
Member

I'd vote for not changing the order that stuff is in the files. It might have been nice if there had been a more consistent ordering from the beginning, but I think this is actually of limited value given that editors have a 'jump to definition' command.

@43081j
Copy link
Contributor Author

43081j commented Jun 6, 2019

thats fair enough.

i agree.

i reverted ordering changes and reverted whitespace changes in template expressions. i also changed the indent rule to a warning instead (as it will error on those still)

@43081j
Copy link
Contributor Author

43081j commented Jun 6, 2019

i've rolled back some more unicode mess and i re-enabled the indent rule as error level (ignored TemplateLiteral direct child nodes and ignored some specific blocks of code).

weirdly the tests no longer pass, not too sure what happened there as the diff is the same as before but cleaner.

edit: the scissors unicode had gone walkies again.. tests fixed

@Rich-Harris Rich-Harris merged commit caebe0d into sveltejs:master Jun 9, 2019
@Rich-Harris
Copy link
Member

thank you!

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

Successfully merging this pull request may close these issues.

Add TypeScript support to ESLint.
4 participants