-
Notifications
You must be signed in to change notification settings - Fork 18
[SBA-02b] migrate from deprecated tslint to typescript-eslint #133
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
base: main
Are you sure you want to change the base?
Conversation
a0489fb to
923919f
Compare
63535a2 to
41df1c5
Compare
923919f to
97ea2d2
Compare
41df1c5 to
f2f808f
Compare
f2f808f to
bd9f16a
Compare
jbrill
left a comment
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.
Great change! Just a few minor ts-related suggestions/thoughts
| export default defineConfig( | ||
| eslint.configs.recommended, | ||
| tseslint.configs.recommended, | ||
| eslintConfigPrettier, |
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.
This might be a good project to add eslint-plugin-security if we wanted.
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.
Are there any specific rules that you think we need? I tried using this on the fully implemented POC branch and it only produced a bunch of false positives on the detect-object-injection rule, like this example which is valid. I don't really see the benefit of adding this if it's going to cause us to have to add a bunch of ignores to the codebase.
| @@ -1,4 +1,4 @@ | |||
| declare var global: any; | |||
| declare const global: unknown; | |||
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.
Thoughts on the following block instead?
import type { WasmGlobal } from './types/lnc';
declare const global: typeof globalThis & {
[key: string]: WasmGlobal | undefined;
};
export interface GoInstance {
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.
This causes TS errors which I couldn't figure out how to get around. I think it's creating a circular dependency on imports, but I'm not sure.
bd9f16a to
169fb35
Compare
|
@jbrill Thanks for the review. I've addressed all of your feedback. This is ready for another pass. |
Summary
The
tslintpackage has been deprecated for many years and has been replaced bytypescript-eslint. This PR migrates the packages and config to the currently recommended linter.There are no functional changes, just some updates to the code to pass the new recommended linter ruleset.
Steps to Test
yarn installto install the new dependenciesyarn lintto confirm all code passes the linter checksyarn buildto ensure the build still worksRelated Issues & Pull Requests
Depends on: