ionic-team / ionic-app-scripts

App Build Scripts for Ionic Projects
http://ionicframework.com/
MIT License
609 stars 300 forks source link

Optimization 'Remove Unused Fonts' not executing #958

Open MarkErik opened 7 years ago

MarkErik commented 7 years ago

Short description of the problem: In my effort to reduce app size (thanks for noting my previous raised issue about landscape splash screens, and sorry for posting it in here instead of the CLI), I have also encountered what I believe is a bug with Optimization Remove Unused Fonts.

The problem is that the logic inside remove-unused-fonts.js doesn't execute. if (context.target === 'cordova') { never evaluates to true. same for: if (context.platform === 'android') {

So when I build e.g. for android, it copies both noto-sans and roboto fonts into the /assets/fonts/ folder. And for iOS it will copy both fonts as well.

Note: I have worked around this issue by commenting out the if-statements and then everything works fine when I run a build with --prod. But this isn't a great solution since this isn't captured in our GitHub (we exclude the node modules directory), and each time I reinstall the ionic app scripts, I need to remember to comment the if-statements out.

What behavior are you expecting? As the logic currently is specified in the remove-unused-fonts.js, I would expect that for all Cordova builds (iOS and android), noto-sans will not be copied on a build using --prod. And for the android platform, roboto would not be copied.

Steps to reproduce:

  1. Delete www/assets (to start clean)
  2. Build app for android using "ionic cordova build android --prod"
  3. See that noto-sans and roboto are copied into the /platforms/android/assets/www/assets/fonts
  4. Fonts also are in the APK (if I unzip to inspect)

Which @ionic/app-scripts version are you using? global packages: @ionic/cli-utils : 1.0.0-rc.0 Cordova CLI : 6.5.0 Ionic CLI : 3.0.0-rc.0

local packages: @ionic/app-scripts : 1.3.6 @ionic/cli-plugin-cordova : 1.0.0-rc.0 @ionic/cli-plugin-ionic-angular : 1.0.0-rc.0 Ionic Framework : ionic-angular 3.1.0

System: Node : v7.7.1 OS : macOS Sierra Xcode : Xcode 8.3.2 Build version 8E2002 ios-deploy : 1.9.0 ios-sim : 5.0.11

githistory commented 7 years ago

confirm seeing the same

gartorware commented 6 years ago

Yep. Problem is that neither context.target nor context.platform are set. Debugging the code seems that config.js is failing when generating the context. This script is searching the target property via argument (as --target?) then via environmnet var.

Using this command ionic build cordova android --prod: In the first part, when doing

var argVal = getArgValue(argFullName, argShortName);
    if (argVal !== null) {
        return argVal;
    }

it seems to not take into account the cordova and android part. As a workaround we can use the environment variables method. If you do this:

export IONIC_TARGET="cordova"
export IONIC_PLATFORM="android"
ionic cordova build android --prod

Then the scripts works correctly. I wonder what side effects could have building without correctly setting these properties.

Sorry for my English.

timler commented 6 years ago

When I added the environment variables (and removed the platforms and www folders), only the ionicicons.wolff2 and ionicicons.wolff fonts were added to the APK assets/www/assets/fonts folder.

If I do the same android prod build (without the environment variables), then I get a full list of Roboto, Noto and Ionic Icons fonts (totalling 2.3 MB).

I started with a blank Ionic 3 project and didn't customize fonts.

It appears as if the font imports in variables.scss are ignored?

@import "roboto";
@import "noto-sans";

Ionic info:

global packages:

    @ionic/cli-utils : 1.4.0
    Cordova CLI      : 7.0.1 
    Ionic CLI        : 3.4.0

local packages:

    @ionic/app-scripts              : 2.0.2
    @ionic/cli-plugin-cordova       : 1.4.1
    @ionic/cli-plugin-ionic-angular : 1.3.2
    Cordova Platforms               : android 6.2.3
    Ionic Framework                 : ionic-angular 3.5.3

System:

    Node       : v6.11.1
    OS         : Linux 3.13
    Xcode      : not installed
    ios-deploy : not installed
    ios-sim    : not installed
    npm        : 3.10.10