chore: update typescript to 5.3.3 #2406
Conversation
spec/types/async-validate.spec.ts
Outdated
| @@ -110,7 +110,7 @@ describe("$async validation and type guards", () => { | |||
| let result: boolean | Promise<Foo> | |||
| if ((result = validate(data))) { | |||
| if (typeof result == "boolean") { | |||
| data.foo.should.equal(1) | |||
| ;(data as any).foo.should.equal(1) | |||
There was a problem hiding this comment.
is there a reason for the leading semicolon?
Also, instead of casting as any, I would cast as Foo, because that's what we expect.
@jasoniangreen are you familiar with this test? I honestly haven't touched the async validation, but this does smell a bit. I feel like it should return either a boolean or a promise. Where is the randomness that means it could return either? I feel like only one of these branches runs, and so we should probably care about that branch?
After upgrading to 5.3 is data unknown? or something else?
There was a problem hiding this comment.
The leading semicolon was prettier.
I will look into the randomness and understand which branch it is. I'm still getting used to the project, so thanks for the idea.
And yes, it is unknown, but like I said, when I downgrade again to 4.9.5 it still shows as unknown, but for some reason it doesn't break the test.
There was a problem hiding this comment.
I wonder if in the downgrade it's a problem with typescript's caching. In general those would be stores in a tsconfig.tsbuildinfo file, but I'm not entirely sure how VC code does caching. If you really wanted to be safe, I'd recommend doing a git clean -xfd and restarting vscode just make sure, but I'm not entirely sure there's much benefit to verifying.
I did look into this a bit and my read is as such:
- sync validators return a boolean for a type guard
- async validators return a promise of the correct type
Therefore, I think this should always return an AsyncValidator not an AnyValidator (the union of both types), and therefore should always execute the else branch, so we probably don't care too much about having to cast in this instance, although I still think the test should probably be an assert rather then a branch. Maybe @epoberezkin can weigh in?
Side note, I realize the semicolon is to prevent the cast from being interpreted as a call, but it's pretty ugly 😕
There was a problem hiding this comment.
Ok, I'll have a look at it with your advice in mind.
but it's pretty ugly
Just to be clear, when I said it was prettier I meant Prettier doing it automatically, not that I thought it looked prettier :D
And thanks for your input, it's really appreciated!
There was a problem hiding this comment.
Yeah, even after git clean -xfd and hard restart of VSCode I still get this.
There was a problem hiding this comment.
That's very strange. I ultimately don't have super strong feelings about this. I'd be worried that the update breaks type guarantees for some people, but I have nothing to gauge for the potential scope of impact of even if this is used in the way the test is validating. As a result, I think I'd ultimately be okay with it, but I understand if @epoberezkin has reservations.
There was a problem hiding this comment.
Therefore, I think this should always return an AsyncValidator not an AnyValidator (the union of both types), and therefore should always execute the else branch, so we probably don't care too much about having to cast in this instance, although I still think the test should probably be an assert rather then a branch. Maybe @epoberezkin can weigh in?
I agree - it indeed should return AsyncValidator
Side note, I realize the semicolon is to prevent the cast from being interpreted as a call, but it's pretty ugly 😕
It's either prettier, or you have to tinker with million of rules in some other thing, but prettier seems to be heading in the same direction...
There was a problem hiding this comment.
on another matter, maybe I'm a bit lost - I thought there were some other changes in types around type utility - or did they become unnecessary? @jasoniangreen
There was a problem hiding this comment.
You're both right @epoberezkin and @erikbrinkman, the result should always be a Promise so I have made the following change. And as for the change in types utility, it is not needed for the upgrade to 5.3.3.
What issue does this pull request resolve?
What changes did you make?
Is there anything that requires more attention while reviewing?