-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Add preliminary linting and fix lint errors #1498
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
Conversation
"serve": "serve dist -p 3000", | ||
"test": "cross-env NODE_ENV=test jest", | ||
"test:nocache": "cross-env NODE_ENV=test jest --no-cache", | ||
"lint": "tsc --noEmit", |
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.
Does this also do JS? This package has the most JS that still needs to be converted to TS
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.
We would have to enable checkJs
as a compiler flag but that throws hundreds of errors atm.
colAValue: string | ||
colBTitle: string | ||
colBValue?: string | ||
colBValue: string | null |
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.
Was this causing type error? We had a discussion a few weeks ago where we're going to try and use foo?
instead of null
as a standard. I think some of the DB types still return null
so is it possible for them to return undefined
instead?
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.
My first instinct was to keep it the same as the db also. But I found that a lot of libraries only annotate their function args with ?
. So you have to end up running code with extra guards like item.myAttribute === null ? someLib(undefined) : someLib(item.myAttribute)
. Any thoughts on how to avoid that?
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 for the TS repo itself but we thought we'd try and follow along https://github.com/Microsoft/TypeScript/wiki/Coding-guidelines#null-and-undefined
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.
We would want to keep it exactly what the underlying type is. E.g if the underlying field is null, then we should make the type of it null. Same goes for undefined. The reasoning is: if you type something as 'undefined' but it's actually 'null', you can get unintended behaviour from strict equality checks since they have to match the exact type. null === undefined
is 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.
So if we want to eliminate the extra guards do you think we'd need to be submitting upstream PR's and use the fork or a monkey patch?
"examples/*" | ||
], | ||
"scripts": { | ||
"lint:all": "yarn workspaces run lint", |
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.
👍
0f926e0
to
3b410c7
Compare
colAValue: string | ||
colBTitle: string | ||
colBValue?: string | ||
colBValue?: 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.
👍
This adds typescript type compilation checking where needed so we can quickly check for project-wide type errors.
Once #1497 is merged, I can rebase this against master if needed.
Future work
Opens up the possibility to integrate per-package
eslint
so we can remove it from the root workspace.Opens up the possibility to run per-package formatting with
prettier
lint:all
which will run the configured linter per project.