[RW-5959][risk=no] Upgrade jest to latest version #4733
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -433,7 +433,10 @@ export const ProfilePage = fp.flow( | |
|
|
||
| const makeProfileInput = ({title, valueKey, isLong = false, ...props}) => { | ||
| let errorText = profile && errors && errors[valueKey]; | ||
| if (valueKey && !Array.isArray(valueKey)) { | ||
| valueKey = [valueKey]; | ||
|
There was a problem hiding this comment. @petesantos can you review this specific change? Based on my reading of the previous code, L444 when the With Alex's change, the test started failing from this somehow. Do you have ideas on what's going on here? I'm still puzzled as to how the current code is functional. There was a problem hiding this comment. This is interesting, it looks like JS and TS handle spread differently. When the TS compiles down to javascript it must do something to prevent strings from getting split apart on a spread. The new code looks more correct and in-line with what I would expect. I will put together a code example demonstrating this issue. What was the specific error? There was a problem hiding this comment. UI unit profile-page test failure: There was a problem hiding this comment. Cool, thanks - it should be possible to set up a more isolated reproducer here (the above failure is highly detached from the cause, and required some trial and error to trace down), I can try and help set this up as well if you'd like. For the purposes of this PR I think it should be fine to merge now. Pete's comment of "The new code looks more correct and in-line with what I would expect" answers what I was looking for, which is to make sure I wasn't missing something obvious and that the change makes sense. There was a problem hiding this comment. So this is the issue: This Typescript code (in version 3.2.z) const checkValueKey = (valueKey) => {
console.log(['test', ...valueKey])
}
checkValueKey('test')compiles to this in javascript: var checkValueKey = function (valueKey) {
console.log(['test'].concat(valueKey));
};
checkValueKey('test');When using However, the same code when compiled in TS 4.2.3 yields: var __spreadArray = (this && this.__spreadArray) || function (to, from) {
for (var i = 0, il = from.length, j = to.length; i < il; i++, j++)
to[j] = from[i];
return to;
};
var checkValueKey = function (valueKey) {
console.log(__spreadArray(['test'], valueKey));
};
checkValueKey('test');Where the There was a problem hiding this comment. Nice find! Thanks. That seems like the likely culprit. Something that's still not obvious to me though is why the typescript version changed with this PR, that seems like an unexpected side effect. My best guess is that changing There was a problem hiding this comment. It looks like This is a caveat we should be aware of - the jest tests will be using a different version of typescript from our code. There was a problem hiding this comment. One more update - I was wondering what our ES target version was, and whether a change to that would prevent substituting the spread operator at all, I don't think there is any browser we are targeting that doesn't support this. return _this.setState(lodash_fp__WEBPACK_IMPORTED_MODULE_1__["set"](['currentProfile'].concat(valueKey), v));and it now compiles to: onChange: v => this.setState(lodash_fp__WEBPACK_IMPORTED_MODULE_1__["set"](['currentProfile', ...valueKey], v)),I am curious as to whether the error popped up as a result of the target change, or the jest upgrade. In other words - is jest using a different typescript version from our build? There was a problem hiding this comment. Alright... Final update... just for a full understanding of what is happening. This is what I tested to confirm the behavior of the tests: There was a problem hiding this comment. I'm not 100% confident but it looks to me that there was one usage of valueKey that this missed: |
||
| } | ||
| if (valueKey && valueKey.length > 1) { | ||
|
Comment on lines
+436
to
+439
There was a problem hiding this comment. The |
||
| errorText = profile && errors && errors[valueKey[1]]; | ||
| } | ||
| const inputProps = { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,16 +11,13 @@ | |
| "emitDecoratorMetadata": true, | ||
| "experimentalDecorators": true, | ||
| "target": "es5", | ||
|
Comment on lines
-14
to
-16
There was a problem hiding this comment. defining |
||
| "lib": [ | ||
| "es2016", | ||
| "es2018.promise", | ||
| "dom" | ||
| ], | ||
| "module": "es2015", | ||
| "baseUrl": "./src", | ||
| "jsx": "react", | ||
| "resolveJsonModule": true | ||
| } | ||
|
|
||
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.
Resolving typescript issue