-
Notifications
You must be signed in to change notification settings - Fork 0
Feature/lint #274
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
Feature/lint #274
Conversation
❌ Deploy Preview for cell-catalog failed. Why did it fail? →
|
| // }, | ||
| plugins: [ | ||
| "gatsby-plugin-react-helmet", | ||
| "gatsby-plugin-react-helmet-async", |
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.
It seems like for SSR the async version of react-helmet is preferred, and is also more maintained. I have yet to confirm what it does, and what this has changed. I just made the linter happy.
The Helmet is an example of something where I am unclear if we are intentionally implementing it or it is a legacy from the template used to start the repo.
| const useDisableWheel = () => { | ||
| useEffect(() => { | ||
| document.addEventListener("wheel", function (event) { | ||
| document.addEventListener("wheel", function () { |
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.
unused
| const DiseaseCatalogPreview = ({ entry, widgetFor }: TemplateProps) => { | ||
| const props = { | ||
| title: entry.getIn(["data", "title"]), | ||
| content: widgetFor("body"), | ||
| footerText: entry.getIn(["data", "footer_text"]), | ||
| }; | ||
|
|
||
| return <DiseaseCatalogTemplate {...(props as any)} />; | ||
| }; | ||
|
|
||
| export default DiseaseCatalogPreview; | ||
| export default DiseaseCatalogPreview; |
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 is a punt, along with the override rules in the eslint config, the preview code is too much to deal with right now.
| const width = useWindowWidth(); | ||
| const isPhone = width < PHONE_BREAKPOINT; | ||
|
|
||
| const suppressRowClickRef = useRef(false); |
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.
needed to pull hooks out of map
| // eslint-disable-next-line @typescript-eslint/no-misused-promises | ||
| onClick={async () => { | ||
| try { | ||
| await navigator.clipboard.writeText(href); | ||
| setShareTooltipText("Copied!"); | ||
| } catch { | ||
| setShareTooltipText("Failed to copy"); | ||
| } finally { | ||
| setTimeout(() => { | ||
| setShareTooltipText(""); | ||
| }, 1000); | ||
| }); | ||
| } |
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.
navigator.clipboard.writeText(href) is async. I made one linting rule happy by adding this try/catch, but added an ignore to the other one for now.
| ): ColumnsType<any> => { | ||
| const columns: ColumnsType<any> = [ |
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.
Tweaking the column typing was one of the more involved changes.
I typed the columns prop in CellLineTable and I'm using the antd type ColumnType and ColumnProps to get the same type-safety we got from this before:
export type CellLineColumns<T> = GetProp<typeof Table<T>, "columns">
Probably more than one way to do this, but everything seems to be working...
| <html lang="en" /> | ||
| <title>{title}</title> | ||
| <meta name="description" content={description} /> | ||
| <HelmetProvider> |
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.
Wrapping in HelmetProvider seems to be the main difference here. As I said in the write up I'm a little shaky on exactly how this all works, would appreciate feedback.
| className={` navbar-start has-text-centered navbar-menu ${ | ||
| isActive && "is-active" | ||
| }`} | ||
| className={` navbar-start has-text-centered navbar-menu`} |
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.
isActive was never reset, so that classname was never applied. Also is this component going to be used? Doesn't seem to be currently.
| setEnv("staging"); | ||
| } | ||
| }, []); | ||
| }, [hostname]); |
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.
Linter didn't like missing dependency in hook
| <Layout> | ||
| <div> | ||
| <h1>NOT FOUND</h1> | ||
| <p>You just hit a route that doesn't exist... the sadness.</p> | ||
| </div> | ||
| </Layout> |
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 we want this?
| coriellLink: string; | ||
| } | ||
| // eslint-disable-next-line | ||
|
|
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 was a little confusing, we have these eslint comments a few places, but were not running eslint... template legacy? They don't seem necessary with the current config.
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.
I have eslint sent to run in my IDE, so that's probably what the origin of these are
| structure, | ||
| isoforms, | ||
| }: GeneNameTemplateProps) => { | ||
| }: GeneFrontMatter) => { |
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.
Linter doesn't like extending a type but not adding anything.
|
FWIW, I'm not sure this branch should really be merged as-is since its so large and failing a CI/CD check. Maybe more of a conversation piece. I could start with a more minimal set of linting rules (no unused vars/imports) and go from there. |
Problem
So this is... big, but apparently our repo is a little messy. T_T
Even with some overrides and some rules silenced, getting
eslintandtypecheckhappy required many, many changes.At the end of the day we will get:
I think it's worth doing and will make our work going forward more sound.
I am going to extensively comment on this PR to try and explain why things are changing if it isn't obvious.
There are a few open questions or things I'm unsure about:
import styles frominstead of using require and destructuring the classnames, do we want to go that direction?A lot of changes are obvious, removing unused stuff and throwing types on things.