Uh oh!
There was an error while loading. Please reload this page.
refactor(tooling): build the lucide gate's two lookup maps lazily, taking rule 2's baseline to zero - #6156
Merged
Merged
Conversation
…t at import
`check-lucide-icon-record-names.mjs` built `keyByComponent` and `kebabByKey` in
two top-level `for` loops, so importing the module for its exports ran them.
That was the sole entry in `check-entry-guard.mjs`'s `KNOWN_IMPORT_UNSAFE`.
The obvious remedy — move the loops behind the entry guard — was measured and
rejected: it leaves the maps empty for importers, so `describeName('BarChart3')`
prints a WRONG diagnosis for a real violation from a gate that still exits 1.
They are now built on first read inside `liveSpellingFor`, memoised. Nothing
runs at module top level, every importer gets what it got before, and the CLI
output is byte-identical. `KNOWN_IMPORT_UNSAFE` goes to zero in the same commit,
because the baseline is shrink-only in both directions.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019b5UBNMtTzKbVtZZGvFuxe
This was referenced Aug 24, 2026
…ide-lazy-maps # Conflicts: # scripts/check-entry-guard.mjs
yinlianghui-tw
marked this pull request as ready for review
August 24, 2026 22:13
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Aug 24, 2026
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 freeto 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.
Fixes#6147
Closes#6092
scripts/check-lucide-icon-record-names.mjsbuilt its two lookup maps in top-levelforloops, so importing the module for its exports ran them. It was the sole entry incheck-entry-guard.mjs'sKNOWN_IMPORT_UNSAFE. They are now built lazily on first read insideliveSpellingFor, memoised, and that baseline goes to zero.With #6145 merged (it emptied
KNOWN_HAND_TYPED_GUARDS) and this PR emptyingKNOWN_IMPORT_UNSAFE, both of #6092's shrink-only baselines are now at zero, which is that card's last open item. The merged-head verdict line says so in one sentence:Everything below was re-measured at the merged head
cbeabc28c, working tree clean.The remedy this is not
The obvious fix — move the loops behind the entry guard — was measured and rejected by #6092's PR 2, and this PR does not revisit that. Guarding them turns rule 2 green and reports its own baseline line as STALE, and fails the importing suite 5 of 25, because the maps are then empty for importers:
describeName('BarChart3')stops naming the live spelling and says "no live key names the same glyph" instead — a wrong diagnosis for a real violation, from a gate that still exits 1. A green baseline bought with a lying error message is worse than an honest debt line.Lazy satisfies both sides: nothing runs at module top level, so rule 2's line can be deleted honestly, and every importer gets exactly what it got before.
Behaviour preservation — the thing that was actually at stake
The rejected ablation broke one specific message, so that message is the measurement. Same probe, on the pre-change tree and at the merged head:
The whole probe output (five calls:
describeNameon a retired alias, a second retired alias, a non-icon, andliveSpellingFortwice) is byte-identical, same sha25694050e0c5ffd…before the change, after the change, and after the merge. It still sayswrite `chart-column`.The CLI leg is byte-identical too — the gate run against
origin/main's version of the file and against this one both produce sha256331ca92069…, exit 0:The import leg prints 0 bytes.
Non-vacuity on the laziness itself
A memoisation that runs eagerly anyway would pass every test above while changing nothing, so the laziness is measured two independent ways. Both were re-run at the merged head, because a merge that resolves cleanly can still change behaviour.
1. A non-mutating probe wraps
Map.prototype.setand counts per phase. The two builds are 1767 + 2025 = 3792 sets, so where they land is unmissable. The same probe was run againstorigin/main's version of the file — which is post-#6145, so it carries theisEntrypointguard and the eager loops (mutation confirmed on disk: top-levelconst keyByComponent0 to 1,function glyphLookups1 to 0,isEntrypoint(import.meta.url)still 1; restored bytrap … EXIT INT TERMwithgit checkout HEAD --):origin/main)importliveSpellingForliveSpellingFordescribeName6147 minus 2355 is exactly 3792. Same total work, moved off the import path — and the probe demonstrably reports an eager tree as eager, so it is not vacuous. Identical numbers before and after the merge.
2. Direct instrumentation of the build body (injected marker confirmed on disk = 1, restored by
trap, marker remaining = 0):The build does not run at import; it runs on the first read; it runs once.
The emptied baseline is still consulted
An empty debt list that had also stopped being read would look identical to this one. Both directions, re-run at the merged head, each confirmed on disk first and each restored by
trap:1 stale KNOWN_IMPORT_UNSAFE entry/entriesorigin/main(eager)const keyByComponent= 1, list still empty1 scripts/ file(s) export bindings AND run on import, naming:243and:245Those are the card's own line numbers, which it stated as correct "on
mainafter #6092's PR 2" — and post-merge they are.The set is kept as an empty
Setrather than deleted, and its comment now records which remedy emptied it — because the rejected one empties it too.The merge, and how the conflict was resolved
#6145 merged at 21:45 and
origin/mainwas merged in (⛔ never rebased, never force-pushed — the push was a fast-forward804afaa66..cbeabc28c).check-lucide-icon-record-names.mjsauto-merged, and it was checked rather than assumed that both edits survived: refactor(tooling): route all 29 scripts/ entry guards through isEntrypoint #6145'simport { isEntrypoint }andconst invokedDirectly = isEntrypoint(import.meta.url)are present,process.argv[1]occurrences are 0, and this PR'smemoisedLookups/glyphLookups()lazy build is present with 0 top-level loop lines. Neither side was taken wholesale.check-entry-guard.mjsconflicted, entirely inside theKNOWN_IMPORT_UNSAFEdoc comment — theconstline itself was already the empty set on both paths. Resolved by keeping the now-true description of an empty list plusrefactor(tooling): route all 29 scripts/ entry guards through isEntrypoint #6145's measured specifics (the 5-of-25 figure and the two quoted diagnoses), so nothing measured was dropped.KNOWN_HAND_TYPED_GUARDS(refactor(tooling): route all 29 scripts/ entry guards through isEntrypoint #6145's,new Map([])) andKNOWN_IMPORT_UNSAFE(this PR's,new Set()) are both empty.git diff origin/main --staton the merged branch is still exactly 2 files, +57/−28 — the merge pulled nothing of anyone else's into this PR's own diff.Verification, each quoting its own verdict line
All at merged head
cbeabc28c; exit codes captured before any pipe.check-lucide-icon-record-names.test.tson its own: 25 passed (25) — before the change, after the change, and after the merge. The same 25 the rejected ablation took to 20.No lint narrowing was needed:
pnpm lint:rootran the full population. Its 28 warnings are all pre-existing —eslint --format jsonover the two changed files reports exactly 2 file objects,errorCount=0andwarningCount=0each, so none of the 28 is in this diff.Control bytes: the repo scanner is green above, and
grep -naPfor the control-byte class over both changed files returns no matches.⛔ Not widened
iconsandiconNamesare themselves top-levelawait import('lucide-react'), so this module is never cheap to import. This PR removes the two loops, not that cost, and deliberately does not restructure the four top-level awaits — that is a separate and larger question, and touching it here would repeat the mistake #6092 stopped to avoid. Both the file's comment and this description say so.Not done, and not owed by this seat: CI convergence. The PM verifies the real gate jobs and lands it; this stays a draft.
Generated by Claude Code