-
-
Notifications
You must be signed in to change notification settings - Fork 799
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
[legacy-framework] Update eslint to v7 in new app template and the blitz repo #853
[legacy-framework] Update eslint to v7 in new app template and the blitz repo #853
Conversation
cc @nettofarah |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pragmaticivan so sorry for the long delay here!
Thank you so much!! Looks good, I just have one question.
Ok now there's a yarn.lock conflict that needs resolved, and then we can merge! Note: this doesn't fully close blitz-js/legacy-framework#661, the one remaining item is to update the eslint and plugin versions in the new app template. If you want to tackle that issue too, you can add that to this PR or do it as a separate PR. Up to you. And of course you can leave it for someone else to do. :) |
cd188b5
to
9cc3bd6
Compare
hi @flybayer, I just fixed the conflicts, rebased the PR, and added the update on template app. |
@pragmaticivan awesome! One final thing which I think you might have overlooked in my last comment, is to also update the eslint plugins in the new app template package.json. |
@@ -47,7 +47,7 @@ | |||
"@typescript-eslint/eslint-plugin": "2.x", | |||
"@typescript-eslint/parser": "2.x", | |||
"babel-eslint": "10.x", | |||
"eslint": "6.x", | |||
"eslint": "7.x", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi @flybayer you meant this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is good, but we also need to update the other eslint-*
packages in this file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it would be also worth updating?:
@typescript-eslint/eslint-plugin 2.x → 3.x
@typescript-eslint/parser 2.x → 3.x
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes! All the eslint related packages
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, thanks!
Awesome, I tested and it works great. Thank you so much!! @all-contributors add @pragmaticivan for code |
I've put up a pull request to add @pragmaticivan! 🎉 |
Closes: blitz-js/legacy-framework#661
Address the root migration part of blitz-js/legacy-framework#661.
What are the changes and their implications?
This PR updates eslint to v7.x.
Follows new recommendation for V7 migration: https://eslint.org/docs/user-guide/migrating-to-7.0.0#default-ignore-patterns
It currently ignores dotfiles but not .eslintrc.js
Checklist