ziofat / material_design_icons_flutter

The Material Design Icons (https://materialdesignicons.com/) Icon pack available as set of Flutter Icons.
MIT License
166 stars 33 forks source link

cannot tree shake when build web #54

Closed Mae623 closed 1 year ago

Mae623 commented 1 year ago

Hello, I come across a question about this package cannot tree shake when build web. The error is ` This application cannot tree shake icons fonts. It has non-constant instances of IconData at the following locations:

The package version I use is 6.0.7096.

flutter doctor information:

[✓] Flutter (Channel master, 3.8.0-14.0.pre.6, on macOS 13.2.1 22D68 darwin-arm64, locale zh-Hans-CN) [✓] Android toolchain - develop for Android devices (Android SDK version 33.0.1) [✓] Xcode - develop for iOS and macOS (Xcode 14.1) [✓] Chrome - develop for the web [✓] Android Studio (version 2022.1) [✓] VS Code (version 1.75.1) [✓] Connected device (4 available)

Wish for your reply!

AlwinFassbender commented 1 year ago

Same issue here. I forked this repository and removed the lines responsible for this error. Result:

Font asset "materialdesignicons-webfont.ttf" was tree-shaken, reducing it from 1261792 to 1019916 bytes (19.2% reduction). Tree-shaking can be disabled by providing the --no-tree-shake-icons flag when building your app.

Which isn't much, but not being able to use the tree-shake option hurts as other icon assets are significantly reduced in size.

Here are the lines that need to be removed:

  static IconData? fromString(String key) {
    int? codePoint = iconMap[MdiIcons.toCamelCase(key)];
    if (codePoint == null) return null;
    return _MdiIconData(codePoint);
  }

  IconData? operator [](String key) {
    int? codePoint = iconMap[MdiIcons.toCamelCase(key)];
    if (codePoint == null) return null;
    return _MdiIconData(codePoint);
  }

Would it be possible to remove support for the method and the operator? Or do you see any other solutions? @ziofat

mono0926 commented 1 year ago

I forked this repository and removed the lines responsible for this error.

I think there are people who would appreciate it if your forked URL is noted. https://github.com/AlwinFassbender/material_design_icons_flutter (commit: https://github.com/AlwinFassbender/material_design_icons_flutter/commit/1a89860752b09ca00073e3f09843a470073f911e )

ziofat commented 1 year ago

@AlwinFassbender This code was introduced for getting icons dynamically, and it was tree-shakable back then as I mentioned in README. Usually if you don't use the operator it should be OK. I have moved on from Flutter and not aware is there any changes related to this, but for me I'm OK with removing this function.

pablojimpas commented 1 year ago

I have moved on from Flutter and not aware is there any changes related to this, but for me I'm OK with removing this function.

Yesterday, Dart 3 and Flutter 3.10 were released, enabling tree-shaking by default on web builds. I found that my build was broken after upgrading because of these two methods. Not the end of the world since tree-shaking can be disabled, but it's a bummer because you lose the massive benefits in bundle size.

Maybe this is something that the Dart tooling has to deal with, since I'm not using those methods in my code anyway, the tooling should be able to infer that and tree-shake appropriately, no?

The28AWG commented 1 year ago

It would be great if the font was adapted for this function. After removing the extra lines, I got the following results: Font asset "materialdesignicons-webfont.ttf" was tree-shaken, reducing it from 1243500 to 1005392 bytes (19.1% reduction). Which is actually unsatisfactory and untrue. I don't use so many icons in a project.

dickermoshe commented 1 year ago

This package is not compatible for Flutter Web 3.10, this should be placed in the description.

bambinoua commented 1 year ago

Until PR #56 merge you may use my repo. ref: tree-shake-issue

thomasgi1 commented 1 year ago

Until PR #56 merge you may use my repo. ref: tree-shake-issue

The fix in your repo does not work. Using your repo I get still

This application cannot tree shake icons fonts. It has non-constant instances of IconData at the following locations:
  - file:///Users/tom/.pub-cache/git/material_design_icons_flutter-35f570079187e3a180e68d2e304a5609a6689ca9/lib/material_design_icons_flutter.dart:7224:12
  - file:///Users/tom/.pub-cache/git/material_design_icons_flutter-35f570079187e3a180e68d2e304a5609a6689ca9/lib/material_design_icons_flutter.dart:7230:12
Target web_release_bundle failed: Exception: Avoid non-constant invocations of IconData or try to build again with --no-tree-shake-icons.
The28AWG commented 1 year ago

@thomasgi1 use ref: tree-shake-issue on dependencies

dickermoshe commented 1 year ago

replace

material_design_icons_flutter: ^6.0.7096

with

material_design_icons_flutter:
  git:
    url: https://github.com/bambinoua/material_design_icons_flutter
    ref: tree-shake-issue
thomasgi1 commented 1 year ago

Thank you for the hint. But now the MdiIcons.* variables are no longer const, which breaks existing projects using those icons.

dickermoshe commented 1 year ago

Also , the icons are not mapped correctly. This PR #56 is broken.

bambinoua commented 1 year ago

The fix in your repo does not work. Using your repo I get still

It should. See the first post of #56. I could compile and run the demo.

bambinoua commented 1 year ago

Also , the icons are not mapped correctly. This PR #56 is broken.

What do you mean?

Oririnal code (first and last are taken)

class MdiIcons {
 static const IconData abTesting = const _MdiIconData(0xf01c9);
 ...;
 static const IconData zodiacVirgo = const _MdiIconData(0xf0a88);
}

Modified code (first and last are taken)

class MdiIcons {
  const MdiIcons();

  static const _values = <String, IconData>{
    'abTesting': _MdiIconData(0xf01c9),
    ...,
    'zodiacVirgo': _MdiIconData(0xf0a88),
  }

  static IconData get abTesting => _values['abTesting']!;
  ...
  static IconData get zodiacVirgo => _values['zodiacVirgo']!;
}

If you found a bug please point the line number.

The28AWG commented 1 year ago

I think the problem is that the icons are no longer a constant, which forces us to change the code in the project. I think it's better to fix it. In addition, the icons are generated according to the template, and you should first change the file update.dart

bambinoua commented 1 year ago

I think the problem is that the icons are no longer a constant, which forces us to change the code in the project.

Yes. I catched the "problem". The example does not have direct mentions of MdiIcon.<name>.

But actially that is not a problem. Yes it is breaking change but new MdiIcons getter is a constant anyway. So you all should do your choice - change the code or do not use font tree shaking.

Blue or red? ;)

In addition, the icons are generated according to the template, and you should first change the file [update.dart]

I did not know this. I just re-map the MdiIcons class. Why do I need to use update.dart? What I did is just refactor the MdiIcons class to allow using tree shake feature. As user of this class I never used the update.dart.

The28AWG commented 1 year ago

In the future, when updating a font version, someone will simply run update.dart and get a new set of icons, and your changes will be overwritten.

The28AWG commented 1 year ago

That is, your change does not solve the problem forever, but just offers a little "here and now" trick, and to solve the "here and now" problem, I simply removed the dependency on the package, downloaded the font file and rewrote the implementation of MdiIcons using only those icons that I needed them in the project, there are not so many of them.

bambinoua commented 1 year ago

@The28AWG I am not agree if that is a trick. Let's leave update.dart apart for a while. All that user needs is just to remove const from MdiIcon. You right, and usually the number of used icons is not so big. So (suppose the PR is merged) the user can use MdiIcon on regular base. The same as flutter team deprecates some styles or functions. We (programmers) modify our code once and continue work.

bambinoua commented 1 year ago

The templates are updated so update.dart will generate correct data.

ziofat commented 1 year ago

Resolved with 7.0.7296, thanks to @bambinoua .