-
Notifications
You must be signed in to change notification settings - Fork 361
Suppress type errors in numeric-input.class.tsx #2433
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
…mittent typechecking errors
|
Size Change: 0 B Total Size: 466 kB ℹ️ View Unchanged
|
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (6efc9c1) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR2433If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.js -t PR2433 |
| focus: () => boolean = () => { | ||
| this.inputRef.current?.focus(); | ||
| // TODO(benchristel): type `inputRef` properly. | ||
| // Typechecking fails here intermittently, probably because of nondeterminism |
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've never had trouble with this, is it possibly a local configuration issue? Are other people seeing this? The fact that TS fails it sometimes seems like a red flag to me. Is there a ticket to follow up on this?
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 have any special local configuration. Are you running pnpm tsc --watch from the command line? Or something else?
I seem to recall that @SonicScrewdriver was able to repro this.
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.
Oh yeah, I was a little confused about this one. It was working perfectly fine on my machine, but I believe other people on the team were seeing some errors. It passed the PR checks fine as well, at the time.
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.
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.
The way that we've defined this ref to bridge between two different components feels wrong to me. I can repro the type check failures by using pnpm tsc --watch and then making a change in this file.
I think what we should do is have the ref defined using an interface with the common functions defined in that interface.
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 fix things for you @benchristel
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.
#2452 has landed. :)
|
Superseded by #2452. |

These type errors appeared intermittently when running
tsc --watch. I suspectthere's a cleaner way to deal with these refs. In the short term, suppressing the
errors in code is preferable to tolerating them.
Issue: none
Test plan:
pnpm tsc -w