Skip to content

perf(icon): skip the registry lookup for names nothing can resolve (#DS-5798) - #2256

Open
nazirgitwork wants to merge 2 commits into
koobiq:mainfrom
nazirgitwork:perf/DS-5798/icon-registry-miss
Open

nazirgitwork wants to merge 2 commits into
koobiq:mainfrom
nazirgitwork:perf/DS-5798/icon-registry-miss

Conversation

@nazirgitwork

Copy link
Copy Markdown

Summary

KbqIcon asks KbqIconRegistry for an inline SVG on every name it gets, including the plain font icons
(kbq-plus_16) that most apps never register. For such a name getNamedSvgIcon returns throwError(...),
so every font icon on the page built an Error (with its stack trace) only to fall back to the font class.
The icon now checks whether the registry can resolve the name before subscribing, and skips the lookup
when it cannot.

List of notable changes

  • KbqIconRegistry.canResolve(name) (@docs-private): true for inline markup, a name registered with
    addSvgIcon*, any name when a resolver is provided, and any name in a namespace that has an icon set.
    These are the same branches getNamedSvgIcon takes, so false means the lookup could only error.
  • KbqIcon.resolveSvgIcon renders the font class right away when canResolve is false.
  • Specs for canResolve (empty registry, named and namespaced icons, icon sets, inline markup, resolvers)
    and a KbqIcon spec checking that an unregistered font icon never calls getNamedSvgIcon.
  • API golden file: the new canResolve member.

What should reviewers focus on

  • canResolve must match the branches of getNamedSvgIcon. If a new way to resolve icons is added later
    without updating canResolve, icons it could resolve will silently render as font icons.
  • It is optimistic for resolvers and icon sets (it says true without calling the resolver or fetching
    the sprite), so behaviour there is unchanged: a miss still errors and falls back to the font class.
  • canResolve is public but hidden from the docs with @docs-private, since KbqIcon lives in the same
    entry point; it could be @internal instead if you prefer to keep it out of the typings.

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 this pull request may close these issues.

1 participant