-
Notifications
You must be signed in to change notification settings - Fork 157
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
[#2151] Update Typescript-related Major Dependencies #2166
Conversation
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.
LGTM! might be better to remove the changes to remove whitespace? i realise this pr was made before #2182 though, so apologies for not noticing this earlier!
@@ -34,15 +38,13 @@ | |||
"as-needed" | |||
], | |||
"no-alert": 0, | |||
"linebreak-style": 0, |
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.
Any reason this was removed?
"error", | ||
{ | ||
"code": 120 | ||
} | ||
], | ||
"prefer-object-spread": 0, | ||
"function-call-argument-newline": 0, |
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.
Same for this? Maybe good to give a one-line reason for everything removed or newly added
frontend/package.json
Outdated
@@ -42,19 +42,18 @@ | |||
}, | |||
"devDependencies": { | |||
"@babel/eslint-parser": "^7.24.1", | |||
"@typescript-eslint/eslint-plugin": "^6.0.0", | |||
"@typescript-eslint/parser": "^6.0.0", |
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.
Both are combined into typescript-eslint
], | ||
"parserOptions": { | ||
"project": ["./tsconfig.json"] | ||
}, | ||
"rules": { | ||
"indent": "off", |
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.
Removed with the addition of @stylistic/disable-legacy
@@ -6,7 +6,11 @@ | |||
"airbnb-base", | |||
"plugin:vue/recommended", | |||
"@vue/typescript", | |||
"plugin:import/typescript" | |||
"plugin:import/typescript", | |||
"plugin:@stylistic/disable-legacy" |
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.
@stylistic/disable-legacy
added to remove old default behaviour that caused bugs and had to be manually turned off.
@@ -15,11 +19,13 @@ | |||
], | |||
"vue/require-prop-types": 0, | |||
"no-param-reassign": 0, | |||
"arrow-parens": [ |
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.
Added namespace.
"error", | ||
"always" | ||
], | ||
"indent": [ |
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.
Added namespace.
@@ -34,15 +38,15 @@ | |||
"as-needed" | |||
], | |||
"no-alert": 0, | |||
"linebreak-style": 0, | |||
"max-len": [ | |||
"@stylistic/linebreak-style": 0, |
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.
Added namespace. Even though linebreak-style
is not enabled by default (due to it being from a plugin), the rule has been added for continuity.
"error", | ||
{ | ||
"code": 120 | ||
} | ||
], | ||
"prefer-object-spread": 0, | ||
"function-call-argument-newline": 0, | ||
"@stylistic/function-call-argument-newline": 0, |
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.
Added namespace. Even though function-call-argument-newline
is not enabled by default (due to it being from a plugin), the rule has been added for continuity.
"error", | ||
2, | ||
{ | ||
"ignoredNodes": [ | ||
"ConditionalExpression" | ||
], | ||
"SwitchCase": 0 |
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.
SwitchCase
is 0 by default.
} | ||
], | ||
"@typescript-eslint/member-delimiter-style": "error", | ||
"@typescript-eslint/type-annotation-spacing": "error", | ||
"@stylistic/member-delimiter-style": "error", |
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.
Changed namespace as part of the rules migration.
@@ -60,25 +64,24 @@ | |||
"files": ["*.ts"], | |||
"extends": [ | |||
"airbnb-typescript/base", | |||
"plugin:@typescript-eslint/recommended" | |||
"plugin:@typescript-eslint/recommended", | |||
"plugin:@stylistic/disable-legacy" |
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.
disable-legacy
had to be re-added here to disable typescript-eslint/recommended
old linting rules from interfering with the new linting rules.
"plugin:import/typescript", | ||
"plugin:@stylistic/disable-legacy" | ||
], | ||
"plugins": [ |
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.
Enable removed rules.
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.
@sopa301 LGTM! Thanks for meticulously looking through all of the updates needed!
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.
LGTM, thanks
@sopa301 Please fix the merge conflicts! We will merge the PR 2 days after the final approval (so tmrw). |
The following links are for previewing this pull request:
|
Fixes #2151.
Proposed commit message
Other information
TypeScript isn't updated to 5.0.0 or later due to a bug in a dependency. However, it doesn't seem to be a major bug, so we can proceed with updating Typescript if it's not a big concern.
#2125 seems to trigger the new
indent
rule quite fiercely. I'm not sure why there is a disparity between the new and oldindent
rules. Perhaps we should discuss which behaviour we prefer?