-
-
Notifications
You must be signed in to change notification settings - Fork 339
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
Enable TypeScript strict mode by ignoring lots of errors #1056
Conversation
- TS 4.4 brings [Improved control flow analysis][1] which eliminates several TS strict errors in `display.ts`. - I had to upgrade ESLint after upgrading TypeScript. [1]: https://devblogs.microsoft.com/typescript/announcing-typescript-4-4/#cfa-aliased-conditions
@pavish @kgodey Since we have a nice lull in the PR queue right now I think it'd be great if we could get this PR merged before any new front end PRs, otherwise we might end up chasing conflicts for a while like we did with #826. I realize we have a couple full-stack PRs that will contain font-end changes, so I'm open to merging this PR either before or after those. |
@seancolsen I'll leave it to you and @pavish to determine merge order; I don't know enough to have an opinion. |
@seancolsen This PR looks good to me! I think it would be best if we could merge #1041 and #1047 first. Both these PRs seem ready on the backend, and we could add frontend changes there and merge them and handle any new ts error in this PR. This PR could then be merged right away. |
@seancolsen It looks like #1047 is not ready yet. I'm approving and merging this PR first. |
|
||
// @ts-ignore: https://github.com/centerofci/mathesar/issues/1055 | ||
// eslint-disable-next-line @typescript-eslint/naming-convention | ||
const getLink__withTypeCoercion: (arg0: unknown) => string = getTabLink; |
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 don't like the name of this const because it goes against everything we've established on the frontend. I believe this particular scenario warrants a discussion and some documentation updates.
Codecov Report
@@ Coverage Diff @@
## master #1056 +/- ##
=======================================
Coverage 92.53% 92.53%
=======================================
Files 108 108
Lines 3830 3830
=======================================
Hits 3544 3544
Misses 286 286
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
Fixes #619
In this PR
display.ts
. I had to upgrade ESLint after upgrading TypeScript.Notes
Review tips
Checklist
Update index.md
).master
branch of the repositoryDeveloper Certificate of Origin
Developer Certificate of Origin