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

@ngtools/webpack - enable sourceMap generates different build #14617

Closed
sod opened this issue May 31, 2019 · 6 comments · Fixed by #16085
Closed

@ngtools/webpack - enable sourceMap generates different build #14617

sod opened this issue May 31, 2019 · 6 comments · Fixed by #16085

Comments

@sod
Copy link

sod commented May 31, 2019

@ngtools/webpack produces different javascript output if you toggle the sourceMap setting. With source maps enabled, the compiler seems to include imports, that shouldn't be there.

Angular: 8.0.0 (but issue is present in 7.2.x as well)
Reproduction: https://github.com/sod/issue-ngtools-14617

Compare both dist-* folders. The only difference in the build is the --source-map flag. In one build the chunk big-dependency is missing and was inlined by webpack.

You can try to build it yourself with either one:

ng build --prod --aot --build-optimizer --source-map
ng build --prod --aot --build-optimizer

If you watch the output with --source-map of the ngtools/webpack loader in https://github.com/angular/angular-cli/blob/master/packages/ngtools/webpack/src/loader.ts#L104, I see for the file https://github.com/sod/issue-ngtools-14617/blob/master/src/app/lorem/lorem.service.ts this output:

import { BigDependencyOptions } from 'src/app/lorem/big-dependency';
import * as i0 from "@angular/core";
var bigDependencyLoader = function () { return import(/* webpackChunkName: "big-dependency" */ '../lorem/big-dependency'); };
var ɵ0 = bigDependencyLoader;
var LoremService = /** @class */ (function () {
    function LoremService() {
    }
    LoremService.prototype.load = function (options) {
        return bigDependencyLoader().then(function (m) { return new m.BigDependency().setOptions(options); });
    };
    LoremService.ngInjectableDef = i0.ɵɵdefineInjectable({ factory: function LoremService_Factory() { return new LoremService(); }, token: LoremService, providedIn: "root" });
    return LoremService;
}());
export { LoremService };
export { ɵ0 };

BigDependencyOptions is an Interface and only used as a type, that import should not be visible in javascript. If you throw the source into typescriptlang, you see that tsc wouldn't include that import as well.

We use an error reporting tool where we can upload the source maps to get better reports. But this behavior increases our build by ~100kb so we compile without source maps right now.

@alan-agius4 alan-agius4 added area: ngtools/webpack freq1: low Only reported by a handful of users who observe it rarely severity3: broken labels Jun 3, 2019
@ngbot ngbot bot added this to the needsTriage milestone Jun 3, 2019
@ngbot ngbot bot modified the milestones: needsTriage, Backlog Jun 6, 2019
@filipesilva
Copy link
Contributor

This seems unintended. I'm not sure why that would change... if you're interested in looking at this bug, I expect https://github.com/angular/angular-cli/blob/master/packages/ngtools/webpack/src/angular_compiler_plugin.ts is the right place.

@filipesilva filipesilva added freq2: medium severity5: regression and removed freq1: low Only reported by a handful of users who observe it rarely severity3: broken labels Jul 2, 2019
@filipesilva filipesilva modified the milestones: Backlog, 8.1.x Jul 2, 2019
@vikerman vikerman modified the milestones: 8.1.x, 9.0.x Oct 17, 2019
@filipesilva filipesilva self-assigned this Nov 5, 2019
@filipesilva
Copy link
Contributor

I'm investigating this problem but I am a bit confused by your report and repro.

lorem.service.ts is part of a lazy chunk and looks like this:

import {BigDependency, BigDependencyOptions} from 'src/app/lorem/big-dependency';
import {Injectable} from '@angular/core';

const bigDependencyLoader = () => import(/* webpackChunkName: "big-dependency" */'../lorem/big-dependency');

@Injectable({providedIn: 'root'})
export class LoremService {
    load(options: BigDependencyOptions): Promise<any> {
        return bigDependencyLoader().then((m: {BigDependency}) => new m.BigDependency().setOptions(options));
    }
}

You have both a static import to big-dependency:

import {BigDependency, BigDependencyOptions} from 'src/app/lorem/big-dependency';

and a dynamic import:

const bigDependencyLoader = () => import(/* webpackChunkName: "big-dependency" */'../lorem/big-dependency');

You expect the direct import to be removed because BigDependencyOptions is an interface and also because BigDependency is unused.

In dist-with-source-map-true we can see the big-dependency.b63b360e949b2fda8373.js chunk. This means that the static import was removed, and only the dynamic import remained, otherwise there would be no lazy chunk.

In dist-with-source-map-false there is no big-dependency.b63b360e949b2fda8373.js chunk. Instead, it's contents are rolled into the other lazy chunk, 5.abbe2b336002b205b922.js. You can verify that is true because 5.abbe2b336002b205b922.js contains two results setOptions: one for the BigDependency class definition and one for it's invocation in LoremService. In dist-with-source-map-true those two results for setOptions are split among the two lazy chunks instead.

So at this point I can see that setting source-maps to false prevents a lazy chunk from being generated.

Yes this looks like a weird problem. But it doesn't impact total bundle size. You mention that this bug increases your build size by ~100kb but I am not seeing that here. What I see instead is that the contents of a lazy chunk were inlined in another chunk. So same total size, but different distribution.

What also confuses me is that you say that you have to disable sourcemaps. But disabling sourcemaps is what causes the inlining. According to this repo, I expected you to have to enable sourcemaps in order to actually have a lazy loaded chunk.

I'll continue looking into why there's a difference of behaviour regardless.

@filipesilva
Copy link
Contributor

filipesilva commented Nov 5, 2019

The reason setting sourcemap makes a difference is related to Build Optimizer. When sourcemaps are not used, we run build optimizer in a less resource-intensive mode. This makes a difference because it uses TypeScript, and TypeScript in its "full" mode removes unused imports. But on the less resource-intensive mode, it doesn't. That's part one of the mystery.

Part two of the mystery is why the unused imports were not removed during the original TypeScript compilation via @ngtools/webpack in the first place. This has to do with code transformations.

We have a code transformation that removes unused angular decorators: https://github.com/angular/angular-cli/blob/master/packages/ngtools/webpack/src/transformers/remove_decorators.ts

This is the original compiled lorem.service.ts without the transformation:

import { __decorate } from "tslib";
import { Injectable } from '@angular/core';
import * as i0 from "@angular/core";
var bigDependencyLoader = function () { return import(/* webpackChunkName: "big-dependency" */ '../lorem/big-dependency'); };
var ɵ0 = bigDependencyLoader;
var LoremService = /** @class */ (function () {
    function LoremService() {
    }
    LoremService.prototype.load = function (options) {
        return bigDependencyLoader().then(function (m) { return new m.BigDependency().setOptions(options); });
    };
    LoremService.ɵprov = i0.ɵɵdefineInjectable({ factory: function LoremService_Factory() { return new LoremService(); }, token: LoremService, providedIn: "root" });
    LoremService = __decorate([
        Injectable({ providedIn: 'root' })
    ], LoremService);
    return LoremService;
}());
export { LoremService };
export { ɵ0 };

And this is how it looks with the transformation:

import { BigDependencyOptions } from 'src/app/lorem/big-dependency';
import * as i0 from "@angular/core";
var bigDependencyLoader = function () { return import(/* webpackChunkName: "big-dependency" */ '../lorem/big-dependency'); };
var ɵ0 = bigDependencyLoader;
var LoremService = /** @class */ (function () {
    function LoremService() {
    }
    LoremService.prototype.load = function (options) {
        return bigDependencyLoader().then(function (m) { return new m.BigDependency().setOptions(options); });
    };
    LoremService.ɵprov = i0.ɵɵdefineInjectable({ factory: function LoremService_Factory() { return new LoremService(); }, token: LoremService, providedIn: "root" });
    return LoremService;
}());
export { LoremService };
export { ɵ0 };

The end result is that __decorate and Injectable are gone. But BigDependencyOptions is weirdly there. This happens because having transformations seems to interfere with how TS removes imports (related, but not the same as microsoft/TypeScript#17552). Also seems somewhat related to angular/angular#21280.

@filipesilva
Copy link
Contributor

#16085 should fix this.

What happened was this:

  • we remove angular decorators as a transformation to speed up builds by removing unneeded symbols
  • when a transform removes references, TS doesn't know it should remove their imports
  • thus we manually add a pass to remove imports whenever we remove or modify nodes
  • in this example our import removing pass identified both BigDependency and import {Injectable} from '@angular/core; for removal
  • removing BigDependency altered the original import {BigDependency, BigDependencyOptions} from 'src/app/lorem/big-dependency'; and caused TypeScript to not identify BigDependencyOptions for removal anymore.

Now we correctly identify type references as not being a usage for an import, for eliding purposes.

@sod
Copy link
Author

sod commented Nov 6, 2019

nice, thank you :)

You mention that this bug increases your build size by ~100kb but I am not seeing that here.

That build size increase was based on our production build - not the reproduction. I guess I should have had added some more files so the alteration in the build was more obvious.

@vikerman vikerman removed this from the v9-candidates milestone Nov 7, 2019
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Dec 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.