Skip to content

fix: support final IconData - #63

Open
MahMoos wants to merge 1 commit into
phosphor-icons:mainfrom
MahMoos:agent/icondata-final-compat
Open

fix: support final IconData#63
MahMoos wants to merge 1 commit into
phosphor-icons:mainfrom
MahMoos:agent/icondata-final-compat

Conversation

@MahMoos

@MahMoos MahMoos commented May 21, 2026

Copy link
Copy Markdown

Summary

  • stop extending IconData now that Flutter marks it final
  • emit generated constants as plain IconData values
  • keep PhosphorIcon duotone rendering through a primary-to-secondary lookup
  • raise the minimum Dart SDK to 2.13 for non-function type aliases

Verification

  • HOME=/tmp flutter analyze --no-pub lib
  • HOME=/tmp flutter test --no-pub (expected: package has no test directory)

@dev-noaman

Copy link
Copy Markdown

Independent verification of this PR, plus one finding that I think matters for whoever
reviews it.

It works in a real app

I pinned this branch (ab72769) via dependency_overrides in
OpenStrap/edge on Flutter 3.44.6, where
phosphor_flutter 2.1.0 fails to compile:

phosphor_icon_data.dart:5:32: Error: The class 'IconData' can't be extended
outside of its library because it's a final class.

That error was taking ~42 test files down with it — whole files failing to compile, not
individual assertions. With this branch:

pass fail
phosphor_flutter 2.1.0 1528 42
this PR 1925 10

The 10 remaining are unrelated pre-existing failures in that project. Duotone icons
render correctly, including through PhosphorIcon's stacked path.

The secondary-codepoint map must stay explicit

The 9k-line _phosphorDuotoneSecondaryIcons map is the obvious thing for a reviewer to
want collapsed into arithmetic, because secondary == primary - 1 looks universal. It
isn't.
Across all 1510 pairs in this diff:

delta count
1 1462
3 21
5 8
4 7
2 3
7 2
6, 8, 9, 11, 12, 14, 33 1 each

48 pairs break the pattern — e.g. 0xe103 → 0xe0fe (Δ5), 0xe105 → 0xe102 (Δ3).
So the explicit map is load-bearing; "simplifying" it would silently mis-pair 48 duotone
icons, which is the kind of bug that ships. Worth a comment in the generator so nobody
optimises it away later.

(It also rules out deriving the secondary at runtime, which would be worse regardless:
non-const IconData defeats icon tree-shaking, and tree-shaking is precisely why
IconData became final.)

Two things worth documenting before merge

1. typedef PhosphorDuotoneIconData = IconData silently changes is checks.
Downstream code that branched on the old subclass now always takes the duotone path:

final glyph = data is PhosphorDuotoneIconData   // now `data is IconData` — always true
    ? PhosphorIcon(data, ...)                    // everything routes here
    : Icon(data, ...);                           // dead code

Output stays correct (PhosphorIcon falls back when there's no secondary), but the
analyzer flags unnecessary_type_check + dead_code, and any caller relying on that
check to discriminate is now wrong. isPhosphorDuotoneIcon() — which this PR adds, and
which is the right replacement — deserves a line in the changelog, and the three
typedefs are probably worth @Deprecated with a pointer to it.

2. Duotone secondaries are no longer individually tree-shakeable. The old design hung
secondary off each icon constant, so referencing one duotone icon pulled in one
secondary. A single global const map means referencing any duotone icon retains
all secondaries. Unavoidable now that IconData can't be subclassed, and it only
affects duotone — but it is a real size regression and should be called out rather than
discovered.

On the other open PRs

For triage, since there are three of these: #66 makes icons plain IconData but drops
duotone's two-layer rendering
, and #62 preserves the rendering but deletes the
PhosphorDuotoneIconData type
, which breaks any runtime is check against it (that is
what made it fail in my project). This PR is the only one that keeps both the type and
the rendering
— which is why I pinned to it.

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.

2 participants