chore: Convert screen.js to TypeScript #1002
Conversation
riotrah
changed the title
src/screen.js
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 9dcdf7f:
|
riotrah
force-pushed
the
pr/ts/src/screen
branch
2 times, most recently
from
8322829 to
ed56210
Compare
src/screen.ts
Outdated
| const initialValue: { | ||
| [key in keyof typeof queries | 'debug' | 'logTestingPlaygroundURL']?: Function | ||
| } = {debug, logTestingPlaygroundURL} |
There was a problem hiding this comment.
Does
| const initialValue: { | |
| [key in keyof typeof queries | 'debug' | 'logTestingPlaygroundURL']?: Function | |
| } = {debug, logTestingPlaygroundURL} | |
| const initialValue = {debug, logTestingPlaygroundURL} as const |
work as well?
src/screen.ts
Outdated
| ? getQueriesForElement(document.body, queries, initialValue) | ||
| : Object.keys(queries).reduce((helpers, key) => { | ||
| : typedKeysOf(queries).reduce<typeof initialValue>((helpers, key) => { |
There was a problem hiding this comment.
There's a reason why Object.keys returns string[] and not an array of literals: https://stackoverflow.com/questions/55012174/why-doesnt-object-keys-return-a-keyof-type-in-typescript. Let's stick with Object.keys and annotate the types properly (casting, @ts-expect-error etc).
There was a problem hiding this comment.
I think in this case perhaps a helpers[key as keyof typeof initialValue] might work best. A @ts-expect-error might work too, however, anyone familiar with the Object.keys caveat may be instantly familiar with the nature and reason of the inline cast.
Though I'm down for whatever! I'll push my proposed change and you can let me if it works.
In addition, doing the cast here makes the explicit typing of initialValue above and the explicit type param passed to reduce here unneccessary as well.
There was a problem hiding this comment.
Double post question: Do y'all care about squashing commits? EG should I squash the aforementioned upcoming commit into the one last pushed?
There was a problem hiding this comment.
Double post question: Do y'all care about squashing commits? EG should I squash the aforementioned upcoming commit into the one last pushed?
During review on github it's usually best to not rebase so that notifications only display the most recent changes. We squash on merge anyway.
There was a problem hiding this comment.
I think in this case perhaps a helpers[key as keyof typeof initialValue] might work best.
Yeah. @ts-expect-error would also hide potential future errors. But let's document why we think the cast is safe and link the Object.keys caveat.
riotrah
force-pushed
the
pr/ts/src/screen
branch
from
ed56210 to
8e9614b
Compare
eps1lon
changed the title
src/screen.js
Codecov Report
@@ Coverage Diff @@
## main #1002 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 25 25
Lines 916 916
Branches 285 284 -1
=========================================
Hits 916 916
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
@all-contributors Add @riotrah for code |
|
I've put up a pull request to add @riotrah! 🎉 |
|
🎉 This PR is included in version 8.2.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
What:
Converting the following to TS:
src/screen.js->src/screen.tsWhy:
#494 outlines motivations and status of project TS conversion, for which this is the start of my contribution.
How:
Renamed file. Found type errors and started to fix them whilst trying to keep runtime logic intact.
Checklist:
docs site N/A
kcd-scriptsbuilds d.ts files from thenpm run buildscript.https://dev.app.trygrok.com/pl/YQHa2AMeAQAeanwo/imO5GqKqQI-sE1jrFFUWjg
It's a mind-map style format for describing code-adjacent metadata and comments. At my company we use these for internal not-documentation for major TS migrations or overhauls and also for PR/MRs to describe changes/questions. We made this beta web viewer version recently after some of wanted to use Flows for our personal OSS contributions. Please let me know if you would prefer that I inline those questions into this issue.