-
Notifications
You must be signed in to change notification settings - Fork 6
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
upgrade node to 20.12.2 #286
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.
Thank you! It looks good. Should we consider removing the package-lock json if we use yarn?
.eslintrc.json
Outdated
"@typescript-eslint/no-unsafe-argument": "off", | ||
"@typescript-eslint/no-unsafe-enum-comparison": "off", | ||
"@typescript-eslint/no-misused-promises": "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.
Just by looking at the name of these tracks, they all sound important. What's the rational of disabling?
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.
As discussed during standup, I checked which rules would be relatively easy to implement:
Re-enabling @typescript-eslint/no-unsafe-enum-comparison
was not too bad so I added that back.
@typescript-eslint/no-unsafe-argument
disables the use of any
types as arguments, which we do all over the place, so I left it disabled
@typescript-eslint/no-misused-promises
checks for multiple conditions, but disallows using async functions as handlers such as many of our onConfirm
s (since they expect to return void). Thus, I have kept the rule, but disabled errors for that condition. Here is the documentation if interested in checking the other conditions.
Created ticket to address this: https://github.com/oncokb/oncokb-pipeline/issues/346 |
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!
"oncokb-styles": "^1.5.0", | ||
"path-browserify": "1.0.1", | ||
"pluralize": "^8.0.0", | ||
"rc-tooltip": "^6.1.2", | ||
"rc-tooltip": "^6.2.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.
Can you verify that the tooltips still look fine? I remember there was some issue related to the misalignment of the arrow under the tooltip.
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
<span> | ||
<TextFormat value={data.createdAt} type="date" format={APP_HISTORY_FORMAT} /> | ||
<span className="mr-2" /> | ||
<span className="me-2" /> |
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.
Looks like that starting using start and end rather than left and right to support RTL layouts?
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.
Yeah, exactly
No description provided.