fix(expect-util): fix comparison of DataView
#14408
Conversation
✅ Deploy Preview for jestjs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
packages/expect-utils/src/utils.ts
Outdated
| let dataViewA = a as DataView; | ||
| let dataViewB = b as DataView; |
There was a problem hiding this comment.
Hm.. I’m not happy to see this cast. Right now it works with the following logic, but if the logic will change TS will fail to catch any errors. Would be useful to rethink this part.
packages/expect-utils/src/utils.ts
Outdated
| if (!(a instanceof ArrayBuffer) || !(b instanceof ArrayBuffer)) { | ||
| return undefined; | ||
| } | ||
| let dataViewA = a as DataView; | ||
| let dataViewB = b as DataView; | ||
|
|
||
| const dataViewA = new DataView(a); | ||
| const dataViewB = new DataView(b); | ||
| if (!(a instanceof DataView && b instanceof DataView)) { | ||
| if (!(a instanceof ArrayBuffer) || !(b instanceof ArrayBuffer)) | ||
| return undefined; | ||
|
|
||
| dataViewA = new DataView(a); | ||
| dataViewB = new DataView(b); | ||
| } |
There was a problem hiding this comment.
Perhaps simply like this (just a quick idea):
let dataViewA: DataView;
let dataViewB: DataView;
if (a instanceof DataView && b instanceof DataView) {
dataViewA = a;
dataViewB = b;
} else if (a instanceof ArrayBuffer && b instanceof ArrayBuffer) {
dataViewA = new DataView(a);
dataViewB = new DataView(b);
} else {
return undefined;
}There was a problem hiding this comment.
Thank you, I've reimplemented it. What do you think?
| let dataViewA = a; | ||
| let dataViewB = b; |
There was a problem hiding this comment.
From semantic point of view it feels wrong to name a variable dataView at the moment it is still not clear if that is a DataView instance.
Also the shape of the logic is very close to: #14408 (comment). Did I miss something in that proposal?
There was a problem hiding this comment.
The logic of your implementation is also correct, except that I wanted to reduce the logical judgments
SimenB
changed the title
DataView
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
Relate PR: vitest-dev/vitest#3703
Test plan
Added