Skip to content
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

chore: update typescript to 5.3.3 #2406

Merged
merged 2 commits into from
Apr 27, 2024
Merged

chore: update typescript to 5.3.3 #2406

merged 2 commits into from
Apr 27, 2024

Conversation

jasoniangreen
Copy link
Collaborator

@jasoniangreen jasoniangreen commented Apr 5, 2024

What issue does this pull request resolve?

  • Updates TypeScript to 5.3.3

What changes did you make?

  • Set a specific version of 5.3.3 and had to cast one test data to any

Is there anything that requires more attention while reviewing?

  • The cast to any needs attention, but even when I revert to master, my VSCode IDE shows that type as unknown anyway. Not sure why it only starts to cause the tests to fail in updated versions of TS, but it's type hasn't changed so the cast seems safe.
  • This is the last version that doesn't have trouble with the Nullable type.

@@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. sync validators return a boolean for a type guard
  2. 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 😕

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, even after git clean -xfd and hard restart of VSCode I still get this.

Screenshot 2024-04-21 at 17 32 32

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@epoberezkin epoberezkin merged commit c64f528 into master Apr 27, 2024
5 checks passed
@epoberezkin epoberezkin deleted the chore-update-ts-5.3.3 branch April 27, 2024 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants