[react-slick] Fix tests assuming instances in ref callbacks are non-nullable#58966
Conversation
|
@eps1lon Thank you for submitting this PR! This is a live comment which I will keep updated. 1 package in this PRCode ReviewsBecause you edited one package and there were no type definition changes, I can help you merge this PR once someone else signs off on it. You can test the changes of this PR in the Playground. Status
All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes. Diagnostic Information: What the bot saw about this PR{
"type": "info",
"now": "-",
"pr_number": 58966,
"author": "eps1lon",
"headCommitOid": "d1e2e8e6b3f64b935b1e39a654cf2ee66d4a6fe8",
"mergeBaseOid": "322b09e7e97422bbeb564159b9ffaafdd1c1827f",
"lastPushDate": "2022-02-24T18:18:03.000Z",
"lastActivityDate": "2022-03-16T07:09:25.000Z",
"mergeOfferDate": "2022-03-15T23:26:34.000Z",
"mergeRequestDate": "2022-03-16T07:09:25.000Z",
"mergeRequestUser": "eps1lon",
"hasMergeConflict": false,
"isFirstContribution": false,
"tooManyFiles": false,
"hugeChange": false,
"popularityLevel": "Popular",
"pkgInfo": [
{
"name": "react-slick",
"kind": "edit",
"files": [
{
"path": "types/react-slick/react-slick-tests.tsx",
"kind": "test"
}
],
"owners": [
"GiedriusGrabauskas",
"r3nya",
"Shannor"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Popular"
}
],
"reviews": [
{
"type": "approved",
"reviewer": "gabritto",
"date": "2022-03-15T23:25:55.000Z",
"isMaintainer": true
}
],
"mainBotCommentID": 1050149455,
"ciResult": "pass"
} |
|
🔔 @GiedriusGrabauskas @r3nya @Shannor — please review this PR in the next few days. Be sure to explicitly select |
|
Re-ping @GiedriusGrabauskas, @r3nya, @Shannor: This PR has been out for over a week, yet I haven't seen any reviews. Could someone please give it some attention? Thanks! |
|
I'm not sure why it pinged me I don't even have write access lol. Though this does look fine to me. |
|
It has been more than two weeks and this PR still has no reviews. I'll bump it to the DT maintainer queue. Thank you for your patience, @eps1lon. (Ping @GiedriusGrabauskas, @r3nya, @Shannor.) |
| }; | ||
|
|
||
| class SliderTest extends React.Component { | ||
| private slider: Slider; |
There was a problem hiding this comment.
IMO this can be written that way, testing/asserting expected types onlyl, more like every day usage (otherwise it starts to look like !newest !Angular with !latest !TS):
diff --git a/types/react-slick/react-slick-tests.tsx b/types/react-slick/react-slick-tests.tsx
index 0813300bda..d0f393fe01 100644
--- a/types/react-slick/react-slick-tests.tsx
+++ b/types/react-slick/react-slick-tests.tsx
@@ -67,12 +67,15 @@ const defaultSettings: Settings = {
};
class SliderTest extends React.Component {
- private slider: Slider | null = null;
+ private slider: Slider;
render() {
return <div>
<Slider
{...defaultSettings}
- ref={(component: Slider | null) => { this.slider = component; }}
+ ref={(component: Slider | null) => {
+ component; // $ExpectType Slider | null
+ this.slider = component!;
+ }}
>
<div><h3>1</h3></div>
<div><h3>2</h3></div>
@@ -82,9 +85,9 @@ class SliderTest extends React.Component {
<div><h3>6</h3></div>
</Slider>
<div style={{ textAlign: "center" }}>
- <button className="button" onClick={(event) => { this.slider!.slickPrev(); }}>Previous</button>
- <button className="button" onClick={(event) => { this.slider!.slickNext(); }}>Next</button>
- <button className="button" onClick={(event) => { this.slider!.slickGoTo(1); }}>First</button>
+ <button className="button" onClick={(event) => { this.slider.slickPrev(); }}>Previous</button>
+ <button className="button" onClick={(event) => { this.slider.slickNext(); }}>Next</button>
+ <button className="button" onClick={(event) => { this.slider.slickGoTo(1); }}>First</button>
</div>
</div>;
}There was a problem hiding this comment.
But this would hide that this.slider.slickPrev is possibly null at runtime. With the ! we have at least some indication that we're cheating here.
I would prefer if we can merge this now, and then you can change the test later if you wish. Otherwise I'd have to change this PR and wait another 2 weeks for this to get merged. For something that, IMO, is just a stylistic preference.
gabritto
left a comment
There was a problem hiding this comment.
I think Peter's comment has a good point on avoiding having multiple ! in the test, but I'll leave it up to you to change it or not (and if you do, I can quickly re-approve).
|
@eps1lon: Everything looks good here. I am ready to merge this PR (at d1e2e8e) on your behalf whenever you think it's ready. If you'd like that to happen, please post a comment saying:
and I'll merge this PR almost instantly. Thanks for helping out! ❤️ (@GiedriusGrabauskas, @r3nya, @Shannor: you can do this too.) |
|
Ready to merge |
Currently instances in ref callbacks are allowed to be nullable due to our bivariance hack. However, during runtime these instances can be null: https://codesandbox.io/s/refs-are-nullable-m44vxx
We'll likely remove the bivariance hack to catch these issues in the future. In the meantime, existing packages and tests should guard against nullable instances regardless.
The issue was first reported in #58464