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

refactor: Improve typings #11

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

zernie
Copy link

@zernie zernie commented Jan 13, 2020

  • Add missing enums.
  • Fix Locale type
  • Fix CardData required fields
  • Fix README code example

This change is Reviewable

@zernie zernie requested a review from beheh January 14, 2020 18:54
@beheh
Copy link
Contributor

beheh commented Jan 14, 2020

So the Readme changes obviously make sense.

I'm conflicted on the typings, because they're closely related to the typings over at https://api.hearthstonejson.com/v1/enums.d.ts and I don't really want to maintain updating this library every single time they change. We've also internally seen that it's usually really helpful to just the same language the game does (e.g. "CORE" over "Basic") to reduce confusion.

As a library consumer why write Race.All when you can just write ALL? Possibly a type union makes more sense here rather than having to come up with custom enum key names (e.g. type Rarity = "COMMON" | "FREE" | "RARE" | ...), and that should also give IDEs their autocomplete.

In any case the library is also a client library that will load metadata at runtime. It's entirely possible you call getLatest at runtime and the returned cards reference a new expansion that wasn't present at install time. So none of the types is necessarily guaranteed, it's more like type CardSet = "CORE | "EXPERT1" | ... | string because it could feasibly be anything!

@zernie
Copy link
Author

zernie commented Jan 15, 2020

@beheh

  • While I agree that some enums are self-explanatory, for others making them a string union would only complicate their usage, e.g. you want to select cards from the Kobolds and Catacombs expansion - how would one know wtf LOOTAPALOOZA is?:) Same goes for Locale, as I had to google the difference between zhCN and zhTW locales.

Besides, using an enum is better for refactoring this library's consumers, I think. Instead of having the codebase littered with somewhat magical looking strings, you'd have a proper enum that you can search for :)

That's understandable. There are 3 enums likely to change with every expansion - CardSet, MultiClassGroup and Mechanics. I can create PRs to update these enums come next expansion (and it would probably be just a few new lines). But in case I'm unavailable, I suggest we simply broaden the relevant fields' types, for example: set: CardSet | string;

@zernie
Copy link
Author

zernie commented Apr 3, 2020

@beheh what do you think?

@zernie zernie changed the title Improve typings refactor: Improve typings Apr 3, 2020
@beheh
Copy link
Contributor

beheh commented Aug 12, 2020

Sorry for the delayed response. I'm not a huge fan of strictly typing these things here, as they do change over time and in the org we have a better way of managing those (enums.d.ts on hearthstonejson) as we don't check for updates to this library every patch.

I also think the optional types should only be very cautiously removed. I'm aware of a few weird helper cards in the db that are missing some of these fields, and at the end of day they are just derived from a plain XML file that could probably omit all fields but the card id.

@beheh beheh force-pushed the master branch 4 times, most recently from 62ec6c1 to b3d612c Compare November 4, 2021 21:47
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.

2 participants