Skip to content

chore: configure eslint#123

Merged
tbouffard merged 5 commits intomaxGraph:developmentfrom
mayorovad:eslint-init
Oct 23, 2022
Merged

chore: configure eslint#123
tbouffard merged 5 commits intomaxGraph:developmentfrom
mayorovad:eslint-init

Conversation

@mayorovad
Copy link
Copy Markdown

@mayorovad mayorovad commented Oct 7, 2022

Summary
Added eslint to project and configured it (#122).

Found 1855 problems (709 errors, 1146 warnings) 🙈 
628 errors and 291 warnings potentially fixable with the `--fix` option.

Need feedback on code-styling rules like comma-dangle and indent. If everyting is seems fine, I can follow up with auto-fixed problems commit.

Description for the changelog
Added eslint

@Kurt29
Copy link
Copy Markdown

Kurt29 commented Oct 7, 2022 •
edited
Loading

@mayorovad I think you should use the prettier plugin for eslint and remove all formatting rules from eslint because afaik we Prettier is used in this project to format the code.

To be honest, I am not sure if prettier is enforced right now, but if it isn't, it probably should be. That would make the Eslint config a lot easier as well.

@mayorovad
Copy link
Copy Markdown
Author

mayorovad commented Oct 7, 2022 •
edited
Loading

Thank you @Kurt29 ! Bumped prettier, added plugin for eslint integration and also added script to run it. Can you check ec59cb8 - is everything looks right?

Kurt29
Kurt29 reviewed Oct 7, 2022
View reviewed changes
Comment thread .eslintrc.json
Comment on lines +26 to +29
"@typescript-eslint/no-explicit-any": "off",
"@typescript-eslint/ban-ts-comment": "off",
"@typescript-eslint/ban-types": "off",
"@typescript-eslint/no-unused-vars": "off"
Copy link
Copy Markdown

@Kurt29 Kurt29 Oct 7, 2022

Choose a reason for hiding this comment

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

I guess these rules are just temporary?

Copy link
Copy Markdown
Author

@mayorovad mayorovad Oct 8, 2022

Choose a reason for hiding this comment

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

Yeah, need to remove them after solving all unnecessary conditions warnings.

@Kurt29
Copy link
Copy Markdown

Kurt29 commented Oct 7, 2022 •
edited
Loading

Thank you @Kurt29 ! Bumped prettier, added plugin for eslint integration and also added script to run it. Can you check ec59cb8 - is everything looks right?

This commit LGTM

But should check that the version bump doesn't affect too much code. Cannot try that out right now for me

Kurt29
Kurt29 reviewed Oct 7, 2022
View reviewed changes
Comment thread tsconfig.json Outdated
{
"compilerOptions": {
"module": "ES2020",
"lib": ["dom", "es5"],
Copy link
Copy Markdown

@Kurt29 Kurt29 Oct 7, 2022

Choose a reason for hiding this comment

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

Shouldn't that lib be es2020?

Suggested change
"lib": ["dom", "es5"],
"lib": ["dom", "es2020"],

Copy link
Copy Markdown
Author

@mayorovad mayorovad Oct 8, 2022

Choose a reason for hiding this comment

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

I think best is es6, because es2020 features currently not used in this code and es5 may lead to some excessive code when transpiling to es6 and higher.

@mayorovad
Copy link
Copy Markdown
Author

mayorovad commented Oct 8, 2022 •
edited
Loading

I'm sorry, this edits is beyond the scope of PR, but it is what it is 😸. Edited base tsconfig.json and changed target from es2020 to es6. Size of transpiled code almost the same (2.35 Mb vs 2.38 Mb), but it will provide better compatibility overall i think. What do you think @tbouffard @Kurt29 ?

@tbouffard
Copy link
Copy Markdown
Member

tbouffard commented Oct 9, 2022

To be honest, I am not sure if prettier is enforced right now, but if it isn't, it probably should be. That would make the Eslint config a lot easier as well.

@Kurt29 Prettier is not enforced but we would like because formatting issues are regularly introduced (for instance, see #88 (review) or #77)
So, if we can have prettier run along with eslint as you suggest, this will be great.

@mayorovad are we forced to change the TypeScript configuration as part of this PR? I can review the eslint configuration but I don't have time for now to check the impact of changing the transpile configuration (what you propose seems ok but I would like more time to discuss with other people of this change)

@mayorovad mayorovad changed the title chore: configure eslint #122 chore: configure eslint Oct 9, 2022
@mayorovad
Copy link
Copy Markdown
Author

mayorovad commented Oct 9, 2022

@tbouffard I checked, with current set of rules in eslint config there is no need for tsconfig changes. In future we can add some rules that requires parser. For now reverted changes in all tsconfig.json!

tbouffard
tbouffard approved these changes Oct 22, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@tbouffard tbouffard left a comment

Choose a reason for hiding this comment

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

Thanks @mayorovad for this PR, this is a great step forward. I am pretty sure that with the upcoming work that will fix all eslint/prettier issue, it will help new contributors.

About the tsconfig change you reverted, I think it is something we should implement so if you have time, feel free to create a new PR about it. I will review it quickly 😄

@tbouffard tbouffard merged commit 3527a77 into maxGraph:development Oct 23, 2022
@tbouffard tbouffard mentioned this pull request Oct 23, 2022
Merged
@mayorovad mayorovad deleted the eslint-init branch October 29, 2022 19:25
@tbouffard tbouffard added the chore Build, CI/CD or repository tasks (issues/PR maintenance, environments, ...) label Nov 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tbouffard tbouffard tbouffard approved these changes

+1 more reviewer

@Kurt29 Kurt29 Kurt29 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

chore Build, CI/CD or repository tasks (issues/PR maintenance, environments, ...)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mayorovad @Kurt29 @tbouffard