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

Improve types #2447

Merged
merged 5 commits into from
Jul 26, 2023
Merged

Improve types #2447

merged 5 commits into from
Jul 26, 2023

Conversation

allmtz
Copy link
Contributor

@allmtz allmtz commented Jul 24, 2023

-Created the UnitDataStructure type to improve type safety when adding a new unit to units.ts

-Narrowed the possible values of CreatureType to only valid combinations of creature.realm + creature.level

@vercel
Copy link

vercel bot commented Jul 24, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
ancientbeast ✅ Ready (Inspect) Visit Preview Jul 26, 2023 5:26am

@ghost
Copy link

ghost commented Jul 24, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@allmtz
Copy link
Contributor Author

allmtz commented Jul 24, 2023

I believe the linter error is due Prettier not supporting the satisfies keyword until version 2.8. The project is using Prettier 2.5.1.

I could try silencing the linter or bumping the version, @DreadKnight which would you prefer?

@andretchen0
Copy link
Contributor

andretchen0 commented Jul 24, 2023

@allmtz, I'm all for bumping Prettier versions if that's what it takes.

But I wonder if this ...

] as const satisfies UnitDataStructure;

... is doing what you want.

satisfies is new to me, so I could be wrong, and if so, apologies in advance.

Edit: It was the as const that confused me. Carry on!

@allmtz
Copy link
Contributor Author

allmtz commented Jul 25, 2023

But I think what this tells TS is "treat this as if it satisfies UnitDataStructure."

I believe satisfies is a little different from as. From the TS handbook:

"The new satisfies operator lets us validate that the type of an expression matches some type, without changing the resulting type of that expression."

It's super useful since it allows for type checking whenever creating a new unit. I checked to make sure there was no casting and doing something like name: 0 throws an error.

But I wonder if this ...

] as const satisfies UnitDataStructure;

... is doing what you want.

Using satisfies lets us keep the literal as const inferences TS makes while also type checking. I played around with using only type definitions without satisfies, but this has the downside of losing all the inferences. For example, Realm in types.ts gets inferred as a string instead of "-" | "A" | "G" | "L" | "S" | "P" | "W" | "E".

@DreadKnight
Copy link
Member

DreadKnight commented Jul 25, 2023

I believe the linter error is due Prettier not supporting the satisfies keyword until version 2.8. The project is using Prettier 2.5.1.

I could try silencing the linter or bumping the version, @DreadKnight which would you prefer?

@allmtz You can bump the dependencies just fine as long as they don't trigger a chain reaction. If that happens you'll have to deal with it as well ideally.

@andretchen0
Copy link
Contributor

But I think what this tells TS is "treat this as if it satisfies UnitDataStructure."

I believe satisfies is a little different from as. From the TS handbook:

"The new satisfies operator lets us validate that the type of an expression matches some type, without changing the resulting type of that expression."

It's super useful since it allows for type checking whenever creating a new unit. I checked to make sure there was no casting and doing something like name: 0 throws an error.

But I wonder if this ...

] as const satisfies UnitDataStructure;

... is doing what you want.

Using satisfies lets us keep the literal as const inferences TS makes while also type checking. I played around with using only type definitions without satisfies, but this has the downside of losing all the inferences. For example, Realm in types.ts gets inferred as a string instead of "-" | "A" | "G" | "L" | "S" | "P" | "W" | "E".

Cool. I look forward to checking it out.

@allmtz
Copy link
Contributor Author

allmtz commented Jul 25, 2023

@DreadKnight, I wanted to bring up my comment in #2441 regarding the usage of '1' versus 1. Before I begin implementing these types in game.ts, I'd appreciate some clarification on which one you prefer to use.

@DreadKnight
Copy link
Member

@DreadKnight, I wanted to bring up my comment in #2441 regarding the usage of '1' versus 1. Before I begin implementing these types in game.ts, I'd appreciate some clarification on which one you prefer to use.

@allmtz My issue before was that there was no consistency between the number, so you had one extra version in your array thingy after. But you could go for the no commas version overall, except the string one for Dark Priests I guess, would simplify editing and such a bit.

@allmtz allmtz marked this pull request as ready for review July 26, 2023 05:29
@DreadKnight DreadKnight merged commit fb54459 into FreezingMoon:master Jul 26, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants