fix(pokeapi): tolerate duplicate upstream resource names - #5
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
pnpm buildfails on every PR, including data-only ones (#4, #3 both hit it):PokeAPI's
/item/?limit=100000now returns 2223 items with exactly one duplicated name:roseli-berryappears as both id 723 (the long-standing entry) and id 2279 (a newly added upstream duplicate row). The name index threw whenever two ids mapped to the same lookup key, so the build aborted. It only reproduces without a local.local/pokeapicache, which is why it hits CI and not local builds.Changes
a4fc3914fix(pokeapi): tolerate duplicate upstream resource namesThe name index now distinguishes two collision classes:
console.warn, soroseli-berryresolves to 723, unchanged from the committed data. The comparison is explicit rather than order-dependent, so it stays stable however upstream orders the list.foo-barvsfoobar) — still throws. That case is genuinely ambiguous: normalization is lossy for those two names and any lookup would be a coin flip.byIdgot a narrow version of the same treatment. It previously threw on any repeated id, including an identical duplicate row (same id and same name) — the same class of benign upstream glitch that broke CI. It now throws only when one id carries two different names, which has no safe resolution. No resource kind has duplicate ids upstream today (ability 373, item 2223, move 937 — zero duplicates).Both maps keep every id resolvable, so
byId.get('2279')still works;enrich-championsrelies on that map for numericchampionsIdlookups.New unit tests cover all six cases against a local stub server, no network involved.
aa2ab161data(champions): add PokeAPI ids for mega stones and new abilitiesRegenerated data, purely additive (+36 lines, zero deletions):
pokeApiIds newly resolved for Mega Stones (clefablite→ 2233, etc.) and two abilities (eelevate→ 312,firemane→ 313) that upstream has since added. No existing id changed. Separated from the fix so the two are reviewable independently.c039e00afix(pokeapi): correct misleading unmatched-resource warningThe unmatched-resource warning claimed "no Champions JSON files were written" while the files were written right after regardless. The message was the wrong half:
nihillightis Champions-exclusive with no PokeAPI counterpart, so it is permanently unmatched, and skipping writes on that would mean never emitting champions JSON at all. Also renamed the private helper from...Errorto...Warning— it is onlyconsole.warn'd, and theErrorname is likely why the stale claim survived a refactor.Verification
Full CI sequence run with
.local/pokeapiabsent and the cache redirected out of tree, so it fetched live PokeAPI exactly as CI does:pnpm format:checkpnpm buildpnpm typecheckpnpm testBuild output now carries the collision instead of dying on it:
Also confirmed the new tests fail against the old code with the exact CI error, and that
a4fc3914stands alone — typecheck, tests andformat:checkall pass with the data commit's changes reverted.Note for reviewers
Building at
a4fc3914regenerates exactly the data inaa2ab161, so apnpm buildat that intermediate commit leaves the two JSON files dirty. That is inherent to splitting regenerated data into its own commit.#4 and #3 need a rebase on
mainafter this lands to pick up the fix.