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

Reduce compiler complexity with ScryfallCards.Any #20

Closed
scarletcs opened this issue Jan 20, 2024 · 2 comments · Fixed by #34
Closed

Reduce compiler complexity with ScryfallCards.Any #20

scarletcs opened this issue Jan 20, 2024 · 2 comments · Fixed by #34
Assignees
Milestone

Comments

@scarletcs
Copy link
Collaborator

While dogfooding on this library with other projects to test the current alpha candidate, I've determined that the ScryfallCard.Any type is a severe performance problem for the compiler.

The current API-types library is built around modular joins because that lends itself to describing the Scryfall API effectively.

Each layout involves twenty-or-more joins, and ScryfallCard.Any is a range of 23 of those. That multiplies out to more than 400 joins. The TS compiler can find working with that to be quite slow. Once that gets added into a system with any additional layers of computational complexity, that can balloon out to thousands or tens of thousands of layers of complexity, and the TS compiler can outright give up.

ScryfallCard definitions need to be replaced so that we no longer refer to cards on a per-layout basis, but instead merge functionally identical definitions together. We can of course also export per-layout types, but they can be derived from that type instead.

@scarletcs scarletcs self-assigned this Jan 20, 2024
@thesilican
Copy link
Contributor

Just some of my 2 cents for this project, this is also related to #10.

I think you may want to consider how much type level encoding you want to have. Currently this project seems to be aiming to create an API with as narrow types as possible, describing as closely as possible the actual set of scryfall API responses within the type system. That's fine, but it comes with some tradeoffs that you are choosing to make.

As a point of example consider src/objects/Cards/Cards.ts:105

  /** A card with the Planar layout. */
  export type Planar = Layout<ScryfallLayout.Planar> & SingleFace & AlwaysOversized;

  /** A card with the Scheme layout. */
  export type Scheme = Layout<ScryfallLayout.Scheme> & SingleFace & AlwaysOversized;

We have the planar and scheme cards having their oversized field set to true. This is useful in the case where I've narrowed down a card to be a planar or scheme type, and now can confidently know that the card is oversized. However, is this fact useful enough to the user to warrant adding a special type for it? Perhaps more importantly, even if this fact is useful, is encoding it at the type level the correct way to communicate it? As a user, I would always expect the oversized field of a card to be a boolean. This is the case, ... except for these 2 special cases where the type is actually the constant true. I would rather the type always be a boolean, and then perhaps a note written somewhere that planar and scheme cards will are always oversized.

For another example, consider the types in src/internal/Primitives.ts.

export type Integer = number;
export type Decimal = number;
export type Uuid = string;
export type Uri = string;
export type IsoDate = string;

Writing type aliases may help the user understand that a string in a type actually represents a uuid, or is formatted like a date. However, it is also an extra layer of indirection the user must incur every time they encounter one of those types. Hmm, this object has a field of type Uuid, what is that? ... lemme check the library code... oh it's just a string. Even worse, if the user is using the type Uuid for another part of their project, there may be issues with name collisions or global namespace cluttering. Again, this is another case where information being described at the type level is unnecessary.

As a counterpoint, I'm currently using scryfall-types in a project that uses the scryfall API. Although the types are a lot more coarse than this library, it provided adequate enough type checking for me to consume the scryfall API, which is all that I care about. As a plus, grokking the types offered by the API was a lot more straightforward since most types were mostly flat. For example the IScryfallCard type was a simple union containing common scryfall types.

My opinion of the current state of the project is that too much information is being encoded at the type level. Yes, some of it does provide narrowing type information for the user in some cases, but it also increases the overall complexity of the project for both the compiler and users.

@scarletcs scarletcs added this to the 1.0.0 Alpha milestone Mar 7, 2024
@scarletcs
Copy link
Collaborator Author

@thesilican Thank you. That's genuinely helpful feedback and insight.

I've had a plan to revise the card system and essentially pull out some of the too-specific guts and I'm going to use that to inform making it much simpler overall.

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 a pull request may close this issue.

2 participants