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
Remove Lint Exceptions: Prop Type Exceptions #10521
Conversation
// eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
const defaultRegisterField = (string) => undefined; | ||
const DEFAULT_PROPS = { | ||
toPreviousStep() {}, | ||
onChange() {}, | ||
registerField() {}, | ||
registerField: defaultRegisterField, |
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.
Could we instead update registerField
to support void
as a supported return type?
) => undefined | RefCallback<HTMLElement>; |
➡️
) => undefined | void | RefCallback<HTMLElement>;
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 removed the inline exceptions, but adding void
didn't seem to work.
Unfortunately adding void
as a return type causes more type issues than it solves. An example:
// `app/javascript/packages/address-search/components/address-input.tsx`
<TextInput
required
ref={registerField('address')} // This is either undefined or `ForwardedRef<HTMLInputElement>` adding 'void' here seems like a bad idea.
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.
Oof. Okay, in that case, maybe we could at least inline the default?
registerField: () => 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.
Much better. For some reason I couldn't get the inline function to work before, but now it works. 👍
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.
Couple minor comments, but LGTM 👍 Glad to be starting to chip away away at some of these exceptions!
import BackButton from './back-button'; | ||
import AnalyticsContext from '../context/analytics'; | ||
import { InPersonContext } from '../context'; | ||
import UploadContext from '../context/upload'; | ||
|
||
function InPersonLocationPostOfficeSearchStep({ onChange, toPreviousStep, registerField }) { | ||
interface InPersonLocationPostOfficeSearchStepProps { |
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.
Elsewhere we extend an interface exposed by FormSteps
which helps avoid repeating the type signatures, and also surfaces additional props that are available. Maybe we should do the same here and in InPersonPrepareStep
and InPersonLocationFullAddressEntryPostOfficeSearchStep
?
identity-idp/app/javascript/packages/document-capture/components/review-issues-step.tsx
Line 39 in c386364
interface ReviewIssuesStepProps extends FormStepComponentProps<ReviewIssuesStepValue> { |
@@ -39,7 +39,7 @@ const USPS_RESPONSE = [ | |||
const DEFAULT_PROPS = { | |||
toPreviousStep() {}, | |||
onChange() {}, | |||
registerField() {}, | |||
registerField: (_string) => 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.
If we're going to list the unused argument, I'd think we should name it the same as it's named in the original type signature (field
). There's also technically an options
parameter as well if we wanted to be exhaustive.
Personally, I don't think it matters much to list the arguments, which is why I left it out in my previous recommendations.
identity-idp/app/javascript/packages/form-steps/form-steps.tsx
Lines 33 to 34 in c386364
field: string, | |
options?: Partial<FormStepRegisterFieldOptions>, |
No description provided.