@pkmn/sets: functionality of `Sets` functions don't match types

[Gudine]

Describe the bug:

The return value of some of the functions in @pkmn/sets don't match the types assigned to them. Specifically:

• Sets.unpackSet's return potentially lacks two properties from PokemonSet: gender and level;
• Sets.importSet returns undefined when given a string containing only spaces;
• Sets.canonicalize throws when given a PokemonSet partial that doesn't have species or moves — the latter in particular can happen when giving it the output of Sets.importSet.

Example:

const set1 = Sets.unpackSet("Espeon|||-||||||||");

console.log(set1.gender, set1.level);
// undefined, undefined

const set2 = Sets.importSet("");

console.log(set2);
// undefined

Sets.canonicalize(Sets.importSet("Tangrowth"), Dex);
// Uncaught TypeError: s.moves is not iterable

Expected behavior:

The functions' parameter and return types should match their implementations.

Additional context:

I'd be willing to open a PR to fix the issues, but I'd have to know whether the types or the implementations should be changed.

[scheibo]

Thanks for taking an interest in this. The challenge here is the upstream Pokemon Showdown's PokemonSet type doesnt really reflect the requirements.

• species and moves are always required
• attempting to import anything which does not contain these (eg an empty string) should throw as illegal
• unpacking the set unfortunately cant fill in the gender/level if unspecified, so these should perhaps be marked as optional (they get filled in during team validation)

The reason these types are pretty loose sadly is that so many of the fields are actually optional that it makes it annoying to use the types because you almost always are going to ! them (why I believe is why upstream these types work the same way they do here - they dont actually type the thing theyre purporting to)

[Gudine]

Well, the species and moves issue could be solved by replacing instances of Partial<PokemonSet> with an actual type, defined as:

type PartialPokemonSet = Pick<PokemonSet, "species" | "moves"> & Partial<Omit<PokemonSet, "species" | "moves">>

And gender and level should then be marked as optional on the main type, but I suppose those are changes that should be made upstream first...

Regardless, I do think Sets.importSet should have undefined added as a return type.

Out of curiosity, is there some sort of authority for how those methods and types should work? I'll take your word for those requirements, but they're not strictly followed in this repository or in the Pokemon Showdown one.

[scheibo]

The challenges here primarily involve naming and PS.

Gender and Level are effectively optional on PS, as they get filled in by the TeamValidator if theyre not present. However, PS marks them as non optional in the PokemonSet definition because it is too lazy to come up with multiple PokemonSet types and pass them around throughout the TeamValidator (ie. PokemonSetButWithRequiredGender, PokemonSetWithRequiredGenderAndLevel, etc).

While pkmn could make PokemonSet more accurate by only marking species and moves as truly required bc everything else can be filled in (ability might be required in gen 3+), it would ruin interop with PS and also make things much more unwieldly. There is a tradeoff between accuracy and usefulness, and PS plays pretty fast and loose with this in favor of conveniece (the TS compiler has gotten a lot better at handling narrowing over the years, but bc PS passes around partially-filled-in PokemonSets all over the place it wouldnt be able to help eliminate many !s).

I do not think we are going to be able to change PokemonSet's definition to be more strict.

Regardless, I do think Sets.importSet should have undefined added as a return type.

I think it should probably throw, not return undefined

Out of curiosity, is there some sort of authority for how those methods and types should work? I'll take your word for those requirements, but they're not strictly followed in this repository or in the Pokemon Showdown one.

The authority is the PS code, as it invented the PokemonSet type. What fields are or are not actually required depends on where the PokemonSet is being used, the only hard guarantee for all potential places the set is handled is that there is a species and at least one move, everything else depends on context

---

just to be clear:

(1) I think it is desirable for types to be accurate at the public API level (2) I think clean interop with PS is important (3) I think there probably should be a MinimalPokemonSet and a FilledPokemonSet type which reflects the two most common types of sets, though (a) the naming leaves a lot to be desired (b) this is very non trivial to implement if you change the type and start dealing with the consequences