-
Notifications
You must be signed in to change notification settings - Fork 993
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
chore: Upgrade ESLint from v7 to v8 #3725
Conversation
I initially thought eslint-plugin-prettier and eslint-plugin-jest-dom needed updates, but after the updates to the others, the issues from these seem to have been resolved. |
New issues that appear under ESLint 8 that weren't there with ESLint 7:
|
Have now resolved all the new linting issues. The changes in packages/forms/src/index.tsx could do with a careful look from someone familiar with the intention of the typing. It wasn't entirely clear what the reason was for 'type' was being omitted and that 'id' not being defined. However, linting and tests both pass. The other changes should be straightforward. |
Requesting a HODL ✋ on this till we merge in the babel changes that are going in #3719 please! |
@@ -89,6 +89,7 @@ interface FieldProps< | |||
| HTMLInputElement = HTMLInputElement | |||
> { | |||
name: string | |||
id?: string |
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.
@jtoar can you confirm this is correct?
@@ -45,7 +45,7 @@ export const generateGraphQLSchema = async () => { | |||
// This tries to clean up the output of those errors. | |||
console.error(e) | |||
console.error(chalk.red('Error parsing SDLs or Schema')) | |||
for (const error of e?.errors) { | |||
for (const error of e.errors ?? []) { |
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.
Nice!
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.
Hey @nzdjb thanks for not only tracking the changes it'd take for us to upgrade to ESLint v8, but then also upgrading us as well! 🚀
I'm good with merging this as is, just had one question about TS in forms. The TS I did in the forms package is definitely not great but I'm seeing one type hint (for type
) now that I feel like shouldn't be there:
Before:
After:
I'd just like to know if you thought it was correct show type as a prop when we do nothing with it in TextAreaField
, etc?
Great work on this! I'm merging now. If there's need to make an adjustment based on @jtoar question above, we can handle in a follow-up PR. Thanks, everyone. 🚀 |
Probably not correct, no, but was the minimal change to resolve the issue. I'll take another bash at it and see what can be done about omitting it. |
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.
@nzdjb Thanks for all the work here! 🔥 While I was working on router stuff I came across a TS error.
I've added my suggestion as a fix, have a look whenever you get the chance. Feel free to include it in your ongoing forms related PR.
Whoops! Sorry about that! 😞 Have added your suggested changes to #3762. |
@nzdjb we have a monthly meetup for Redwood Contributors, and it would be a pleasure to add you to the group invite. (Read more about Redwood Contributors here.) Could you send me a DM on the Redwood Forums? Or any other preferred way to connect? |
From #3623
Work remaining:
upgrade eslint-plugin-prettier (once released)upgrade eslint-plugin-jest-dom (once released)Release Notes