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.

Sign up for GitHub

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

Jump to bottom

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8f3b729
chore: update typescript to 5.3.3
jasoniangreen Apr 5, 2024
12bfffa
test: result should always be a Promise
jasoniangreen Apr 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Expand Up @@ -104,7 +104,7 @@
"rollup-plugin-terser": "^7.0.2",
"ts-node": "^10.9.2",
"tsify": "^5.0.4",
"typescript": "^4.9.5"
"typescript": "5.3.3"
},
"collective": {
"type": "opencollective",
Expand Down
2 changes: 1 addition & 1 deletion spec/types/async-validate.spec.ts
View file
Open in desktop
Expand Up @@ -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.

} else {
await result.then((_data) => _data.foo.should.equal(1))
}
Expand Down