Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[flutter_tools] enable tree-shake-icons by default for non-web targets #56633

Merged

Conversation

jonahwilliams
Copy link
Member

@jonahwilliams jonahwilliams commented May 8, 2020

Description

Enable tree-shake-icons by default for non-web targets. Make missing fonts a status print

@fluttergithubbot fluttergithubbot added tool Affects the "flutter" command-line tool. See also t: labels. work in progress; do not review labels May 8, 2020
@jonahwilliams jonahwilliams requested a review from dnfield May 8, 2020 16:16
@jonahwilliams jonahwilliams marked this pull request as ready for review May 8, 2020 16:16
Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

To include some of our offline discussion:

This can happen if kernel compilation with AOT and TFA still has const IconData references to a font that is not being bundled with the application - e.g. a package that tries to dynamically switch between Material and Cupertino icons, but the application does not include the Cupertino icon font. While it is possible that this will cause runtime issues, things are no worse in this case than they would be without the icon shaking, since either way the developer is chosing to not include the font with the bundle. It is also possible that an application will load the font dynamically at runtime, e.g. by downloading it from the web.

We discussed trying to provide a better error message here that tells you line/package numbers for the const reference. Unfortunately, the AST nodes we get from package:kernel do not provide good linkages there, and it's not even clear if that would be possible with const cannonicalization. Thus, we'll just log a status when the font is missing.

@pengchen-ui
Copy link

最新版本的sdk 打包错误 说是非常量实例,可有解决办法

@dnfield
Copy link
Contributor

dnfield commented Aug 14, 2020

Based on what Google translate tells me, the question is:

The latest version of the SDK packaging error is said to be a non-constant instance, there is a solution

The solution is to either use --no-tree-shake-icons or to avoid using non-constant instances of IconData :)

@pengchen-ui
Copy link

谢谢,flutter 很棒

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants