-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Fix unsafe ref usage #4612
Fix unsafe ref usage #4612
Conversation
return value != null && typeof (value as IRefObject<T>).current !== "undefined"; | ||
return value != null && typeof value !== "function"; |
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.
it is valid for RefObject.current
to be undefined
when using refs for something other than HTML elements. This pattern is much more common when using functional components. While Blueprint doesn't really use FCs yet, and the type constraint here should prevent it, this change helps future proof.
* Assign the given ref to a target, either a React ref object or a callback which takes the ref as its first argument. | ||
*/ | ||
export function setRef<T extends HTMLElement>(refTarget: IRef<T> | undefined | null, ref: T | null): void { | ||
if (isRefObject<T>(refTarget)) { |
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 type guards here prevent this from being called with undefined | null
, so I removed the undefined
check
@@ -78,25 +71,10 @@ export function setRef<T extends HTMLElement>(refTarget: IRef<T> | undefined, re | |||
export function refHandler<T extends HTMLElement, K extends string>( | |||
refTargetParent: { [k in K]: T | null }, | |||
refTargetKey: K, | |||
): IRefCallback<T>; | |||
export function refHandler<T extends HTMLElement, K extends string>( | |||
refTargetParent: { [k in K]: T | IRefObject<T> | 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.
This type made it pretty easy to cause runtime errors since it does not require the passed type to include IRefObject<T>
. The problem here being that k
here should be contravariant, but since typescript doesn't allow for this it defaults to being covariant.
This kind of typing issue is pretty common with refs and Typescript, and leads to things like the bivariance hack in the base react types.
More detail here: #4611
@@ -212,7 +211,7 @@ export class DateInput extends AbstractPureComponent2<IDateInputProps, IDateInpu | |||
if ( | |||
e.key === "Tab" && | |||
!e.shiftKey && | |||
this.lastTabbableElement.classList.contains(Classes.DATEPICKER_DAY) | |||
this.lastTabbableElement?.classList.contains(Classes.DATEPICKER_DAY) |
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.
TBH, I'm not sure why the type checker wasn't catching this null call here. It might be worth doing a deeper pass of all current usage of refs to make sure they are properly null checked.
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.
not all packages use strict null checks, unfortunately. this one has that compiler option turned off at the moment.
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.
gotcha, makes sense. I'm so used to having that compiler option turned on, I forgot TS doesn't check that by default haha
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.
lgtm aside from the breaking change
Fixes #4611
Checklist
Changes proposed in this pull request:
This changes a couple aspects of ref usage.
refHandler
always creates a ref callback and always assigns an element to the local target.refHandler
(more) type safe to usenull
, according to https://reactjs.org/docs/refs-and-the-dom.html#caveats-with-callback-refsReviewers should focus on:
Changes to the ref utility functions and component lifecycle methods.