8000 Explore `IconData` being marked `final` by dcharkes · Pull Request #297 · fluttercommunity/font_awesome_flutter · GitHub
[go: up one dir, main page]
Skip to content

Explore IconData being marked final#297

Merged
michaelspiss merged 1 commit intofluttercommunity:masterfrom
dcharkes:explore-icon-data-final
Mar 9, 2026
Merged

Explore IconData being marked final#297
michaelspiss merged 1 commit intofluttercommunity:masterfrom
dcharkes:explore-icon-data-final

Conversation

@dcharkes
Copy link
Contributor
@dcharkes dcharkes commented Feb 16, 2026

PR Description: Migration to FaIconData Wrapper

Summary

This PR transitions the font_awesome_flutter package from an inheritance-based icon model to a composition-based "Wrapper" model. This change is necessitated by the upcoming Flutter framework change where IconData is marked as final (flutter/flutter#181342).

By introducing FaIconData, we also solve a long-standing issue where users would accidentally use Font Awesome icons with the standard Flutter Icon widget, leading to incorrect rendering of rectangular icons.

Breaking Changes

  1. Icon Type Change: All icons in FontAwesomeIcons now return FaIconData instead of IconData. FaIconData no longer implements or extends IconData.
  2. Widget Strictness: The FaIcon widget now strictly requires FaIconData. It can no longer be used with standard IconData (e.g., Icons.add), and the standard Icon widget can no longer be used with Font Awesome icons.
  3. Testing Finders: Flutter finders like find.byIcon and find.widgetWithIcon look at the Icon.icon property. Since FaIcon passes the underlying data to the framework, tests must be updated to match against the raw data: find.byIcon(FontAwesomeIcons.gamepad.data).

Technical Details

  • FaIconData Wrapper: A new @immutable final class that wraps a standard IconData instance.
  • FaIcon Refactor:
    • Updated to store FaIconData? _icon.
    • Uses super(null) in the constructor to allow for const initialization while avoiding invalid property access on parameters.
    • Overrides IconData? get icon => _icon?.data to remain compatible with the Flutter framework's layout and testing machinery.
  • Generator Update: Updated util/lib/main.dart to generate icons using the new wrapper and updated Font Awesome to version 7.2.0.

Migration Guide

In Widgets

Before:

FaIcon(FontAwesomeIcons.user)

After:

FaIcon(FontAwesomeIcons.user) // Nothing changed

In Tests

Before:

expect(find.byIcon(FontAwesomeIcons.user), findsOneWidget);

After:

expect(find.byIcon(FontAwesomeIcons.user.data), findsOneWidget);

Versioning

  • Bumped major version to 11.0.0-wip to reflect these significant breaking changes.

@dcharkes
Copy link
Contributor Author
dcharkes commented Feb 16, 2026

@michaelspiss I have played around a bit with the first option from flutter/flutter#181342 (comment) (as the second option is only an option after we release the new technology which allows packages to tree-shake their own assets).

If I understand it correctly, the correct use was to use FaIcons with one of your custom IconDatas, in user code that should stay identical, no braking change.

But the tests of users require a breaking change for users to access the .data field.

This PR makes the typing stricter, so non font-awesome icons can no longer be used with icon data that is not font-awesome, and font-awesome icon data can no longer be used with a non font-awesome icon. I the first one would have worked properly before, but the latter one would have caused issues with the non-square icons. So the latter one is hopefully an improvement.

I am not very familiar with how your package is being used. So maybe this PR doesn't work at all. Please enlighten me if this is the case, and we'll go back to the drawing board!

@dcharkes
Copy link
Contributor Author
dcharkes commented Feb 25, 2026

Friendly ping @michaelspiss

@michaelspiss
Copy link
Collaborator
michaelspiss commented Mar 1, 2026

Hey @dcharkes - sorry for the delays, I am currently very limited with the time I am able to spend on OSS. I'll try to come back to you faster so you can proceed.

Your PR certainly looks like it would work in our context. Especially the package's icons only working with FaIcon is a welcome improvement as it completely eliminates this pitfall.
However, if I understand correctly, if flutter decides to implement the rectangular icons proposed in flutter/flutter#99830 in the future, they wouldn't work with the font awesome icons?

It's good to see that the main user code stays completely untouched and the breaking change for tests is more than reasonable only being a single sed substitute fix for the whole project.

@dcharkes
Copy link
Contributor Author
dcharkes commented Mar 2, 2026 •
edited
Loading

Hey @dcharkes - sorry for the delays, I am currently very limited with the time I am able to spend on OSS. I'll try to come back to you faster so you can proceed.

Thanks @michaelspiss! 🙏

However, if I understand correctly, if flutter decides to implement the rectangular icons proposed in flutter/flutter#99830 in the future, they wouldn't work with the font awesome icons?

Yes, if Flutter addresses that issue, then the desired pattern would be the one in flutter/website#13005 instead. You use your own custom wrapper class where you nest static consts, but the static const instances would be IconData.

If that happens in the future, the tests would have to be migrated back again. (And you'd probably mark FaIcon as deprecated so that people can move over to Icon.)

Your PR certainly looks like it would work in our context. Especially the package's icons only working with FaIcon is a welcome improvement as it completely eliminates this pitfall.

It's good to see that the main user code stays completely untouched and the breaking change for tests is more than reasonable only being a single sed substitute fix for the whole project.

Great! What is your preferred way of moving forward? Do you want to review this PR and land it? Or would you prefer incorporating the breaking change in a different way on this repo before I make the breaking changes upstream in Flutter?

@michaelspiss
Copy link
Collaborator
michaelspiss commented Mar 3, 2026

Thanks for the input!

Your PR already looks great - I'd look over it once again in a review and publish it by the end of the week

@dcharkes dcharkes mentioned this pull request Mar 3, 2026
Draft
@michaelspiss michaelspiss marked this pull request as ready for review March 9, 2026 05:45
@michaelspiss michaelspiss merged commit 8848ebf into fluttercommunity:master Mar 9, 2026
@michaelspiss
Copy link
Collaborator
michaelspiss commented Mar 9, 2026

Hey @dcharkes! Fyi, I just released version 11 with your changes

@michaelspiss
Copy link
Collaborator
michaelspiss commented Mar 9, 2026

Thanks for the help, much appreciated!

@dcharkes
Copy link
Contributor Author
dcharkes commented Mar 9, 2026

Thanks for the help, much appreciated!

You're welcome!

This was referenced Mar 10, 2026
Draft
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@dcharkes @michaelspiss
0