angular / angular

Deliver web apps with confidence 🚀
https://angular.dev
MIT License
94.98k stars 24.84k forks source link

[RC5]: Minified bundle breaks #10618

Closed antonybudianto closed 7 years ago

antonybudianto commented 7 years ago

I'm submitting a ... (check one with "x")

[x] bug report
[ ] feature request
[ ] support request => Please do not submit support request here, instead see https://github.com/angular/angular/blob/master/CONTRIBUTING.md#question

Current behavior Minified bundle breaks, but SOLVED temporarily by either:

Error traces:

lib-2cf12bf509.js:7 Unhandled Promise rejection: Template parse errors:
Can't bind to 'brand' since it isn't a known property of 'as-navbar'.
1. If 'as-navbar' is an Angular component and it has 'brand' input, then verify that it is part of this module.
2. If 'as-navbar' is a Web Component then add "CUSTOM_ELEMENTS_SCHEMA" to the '@NgModule.schema' of this component to suppress this message.
 ("<as-navbar [ERROR ->][brand]="appBrand"></as-navbar>
<div class="container" style="margin-top: 100px;">
    <router-outlet></"): a@0:11 ; Zone: <root> ; Task: Promise.then ; Value:

Expected/desired behavior The minified bundle should work as in RC4

Reproduction of the problem https://github.com/OasisDigital/rc5-declaration-order

npm install

// try run in dev, it works well now
npm start

// try run in prod, the bundle created but break
npm run serve-build

// now try changing the mangle option and retry, it works!

What is the expected behavior? Should work as in RC4

What is the motivation / use case for changing the behavior?

Please tell us about your environment:

antonybudianto commented 7 years ago

Solved: The problem is the order of the components in declarations field, but it's weird it's only happening in bundled-code, I need early feedback on that when in development

kylecordes commented 7 years ago

Experienced same problem and got up and running with the same solution.

I am not, as of this moment, particularly a fan of this new behavior :-) and I heartily encourage the team to document it in the change log under "breaking changes".

vthinkxie commented 7 years ago

+1

robertoforlani commented 7 years ago

+1

robertoforlani commented 7 years ago

@antonybudianto Which order did you change ?

kylecordes commented 7 years ago

@robertoforlani Hopefully someone will have time to write a comprehensive explanation soon. In the meantime here is what I can do in a couple of minutes.

In order to obtain a warning free and error-free working "production" build, you need to edit the NgModule app module definition for your program, specifically the declarations array. This array should contain a list of all of the components and directives in your application.

Now for the harder part. You must perform a "topological sort" of this list, or for those who didn't study computer science formally or recently, you need to rearrange the order of this list such that the components are in reverse order of use, with the components used most "deeply" in your component hierarchy, listed first.

For example, consider if you had five components in your program, A B C D E. If for example component A used component B in its template, and component B used component C in its template, and so on, then the dependencies between these components are A->B, B->C, C->D, D->E, E->F. In this case the correct order to list them in the declarations would be declarations: [E, D, C, B, A].

Fortunately, in most applications there is not such a deep dependency graph among all components. In many cases you will make this error go away just by editing that declarations list to (1) list all your components, and (2) list your directives and fine-grained small components the beginning, as a heuristic.

cristimusat commented 7 years ago

Experienced almost the same problem.

I have 2 directives (A and B) and 1 component (C). if the order in declarations is [A, B, C], I get the error above for @Input in directive B. if the order in declarations is [B, A, C], I get the error above for @Input in directive A. A and B don't share any dependencies. C is using both A and B. This happens only for bundled build. I am using cli version 1.0.0-beta.10

naomiblack commented 7 years ago

Thanks all -- this seems like a bug. @IgorMinar is investigating.

IgorMinar commented 7 years ago

I'm having hard time reproducing the problem. Can someone provide a repro?

I tried @antonybudianto's repo but $(npm bin)/gulp serve-build fails on an http 404 from a test, once I comment out the test gulp task, the app builds and runs without any problems.

To clarify: the order of declarations should not matter. If changing the order changes the behavior then that's a bug that we'll fix.

IgorMinar commented 7 years ago

I created https://github.com/IgorMinar/declarations-bug-repro with cli but even that repo doesn’t repro the issue.

could someone clone it and modify it to repro the issue? thanks

IgorMinar commented 7 years ago

I got a repro from @kylecordes:

https://github.com/OasisDigital/rc5-declaration-order

See comments in this file about declarations order.  https://github.com/OasisDigital/rc5-declaration-order/blob/master/src/app/app.module.ts

cristimusat commented 7 years ago

I managed to fix my application. The problem was that the 2 components weren't using moduleId: module.id in the @Component declaration. Once I added moduleId: module.id prod build is ok.

antonybudianto commented 7 years ago

@IgorMinar , sorry yesterday I pushed some commits to the branch, it should work as expected. Turned out my AppComponent and NavbarComponent are declared on same module (AppModule), then app.html use navbar component, when bundled, it breaks. and I tried to make navbar into a module itself which AppModule imports, and it solved.

kylecordes commented 7 years ago

If anyone wants to generate an arbitrarily large application to help prove out tools (and reveal issues like this one), I just updated my "angular2-stress-test" for rc.5 and NgModule:

https://www.npmjs.com/package/angular2-stress-test

npm install -g angular2-stress-test
cd directory-with-your-components-in-it
angular2-stress-test 500
sirajc commented 7 years ago

I faced similar issue in bundled release for https://sirajc.github.io/angular2-labs/ I had to add NavbarComponent to bootstrap array bootstrap: [ AppComponent, NavbarComponent ] while deploying, whereas locally during dev I had only bootstrap: [ AppComponent] I encountered this issue few days ago while working on master branch, Thought this is the intended behavior

oocx commented 7 years ago

I also have this problem. Changing the declarations order did not fix it for me.

cristimusat commented 7 years ago

@oocx try putting moduleId:module.id in each component

oocx commented 7 years ago

Fixed it - I had this error in my unit tests, so I had to fix the declarations in my TestBed.configureTestingModule call instead of my app module.

gnujeremie commented 7 years ago

Same problem here, solved it with the mangle option set to false.

ediri commented 7 years ago

Same problem here the fix from @gnujeremie solved it for me to!

return builder.buildStatic('build/src/js/main.js', 'build/app.js', {minify: true, mangle: false});

micaelmbagira commented 7 years ago

Same problem, also happening when using Angular 2 with JavaScript ES5 (even with the normal version, not bundled).

kylecordes commented 7 years ago

I updates the repro repo:

https://github.com/OasisDigital/rc5-declaration-order

to the latest CLI webpack.2, to verify the problem still occurs... as it is not clear to me whether this is really a core problem, or somehow a problem added by webpack or CLI. Yes, problem still occurs.

Sznapsollo commented 7 years ago

I have exactly same problem since updating to Angular2 RC.5. In RC4 all was working very well. Not uglified bundle works well. Also uglified bundle but without --mangle works well but it weights slightly more that it would with mangle

There are two workarounds that were also mentioned here:

  1. reorder things in declarations - which really does not always works because some components might use one another in both ways in which case this approach would not help. However because this helps in some cases then I would suspect that uglified package puts definitions according to order in declarations.
  2. uglify without --mangle which producees minified package but slightly bigger than it would be with mangle from what I observed.

I hope it gets solved. In RC.4 all was working fine in my case with same components and same version of uglifyJS.

Angular 2.0.0-rc.5 browsers-all typescript: 1.8.10 uglifyJS: 2.4.10

Petitt88 commented 7 years ago

I too experience this issue. My temporary solution was to disable minifying. Waiting for rc.6.

KamelJabber commented 7 years ago

Note: I added moduleId: module.id and it made things worse. The minified javascript isn't loading anything except a useless error 'home.js:260 Uncaught TypeError: e.match is not a function'

Going to try to add moduleId back but use NgModule?

Then try playing with the order of the declarations?

Is there an actual fix or just keep trying until it works for me?

Update:

"SOLUTION": The solution for me was to remove moduleId: module.id and re-arrange the order of the components in the declarations with NgModule.

Now to figure out why it's not working with moduleId: module.id ?

Update II: Yah, my time is up for they day...webpack -p is really not liking moduleId at all! :( Moreover, it looks like it never has? bleh

sod commented 7 years ago

Got a reproducable punkr http://plnkr.co/edit/xrFVK1K0OHkKXj1ERYgj?p=preview

Code

import {NgModule, Component, Input} from '@angular/core';
import {BrowserModule} from '@angular/platform-browser';

var ComponentA = function() {
    @Component({
        selector: 'component-a',
        template: `component-a <component-b [property]="1"></component-b>`
    })
    class Foo {} // <- this class is named Foo
    return Foo;
}();

var ComponentB = function() {
    @Component({
        selector: 'component-b',
        template: 'component-b property: {{ property }}'
    })
    class Foo { // <- And this class is named Foo, rename it to Foo2 and it works
        @Input() property: string;
    }
    return Foo;
}();

@NgModule({
    imports: [
        BrowserModule,
    ],
    declarations: [
        ComponentA,
        ComponentB,
    ],
    bootstrap: [
        ComponentA,
    ],
})
export class TestModule {}

Error: image

Renaming one of the class Foo to something else solves the error. I guess something along the pipeline caches by .name. And class names can collide if mangled.

thecritic commented 7 years ago

Same here. I'm using Webpack (https://github.com/AngularClass/angular2-webpack-starter) and the solutions proposed here seem very overkill at first glance. No matter how I order my declarations, I end up getting the same error. Also, while I wouldn't call those proposals hacky, they seem far from usable in a business-critical application.

Regardless, with all due respect to the Angular2 team it's beyond me how this could have made it into RC5. Also, rolling out NgModules at this stage IMHO seems like a very controversial move.

ediri commented 7 years ago

@thecritic using a RCx for a business-critical application is borderline... But that's none of my business... ;)

gnujeremie commented 7 years ago

@ediri @thecritic i do agree with @ediri , that's not the kind of changes supposed to appear in a RC.

Skuriles commented 7 years ago

Same here. Using systemjs and systemjsBuilder. Temporary solution: I used gulp-uglify to bundle my files at the end of build process. Removed this pipe and it works. Using SystemJs without systemjsBuilder (for dev: directly mapped to the node_modules folder) it was no problem.

So it looks more for a bundling/minifiying/uglifying problem than implicit ordering to me...but we'll see

dzonatan commented 7 years ago

A lot of saying that "ordering declarations in correct order solves the problem". But what about modules?? Just look at this:

import { BrowserModule } from '@angular/platform-browser';
import { NgModule, Component } from '@angular/core';

/** FOO MODULE **/

@Component({ selector: 'foo', template: 'I am foo!' })
export class FooComponent { }

@NgModule({ declarations: [FooComponent], exports: [FooComponent] })
export class FooModule { }

/** BAR MODULE **/

@Component({ selector: 'bar', template: 'I am bar!' })
export class BarComponent { }

@NgModule({ declarations: [BarComponent], exports: [BarComponent] })
export class BarModule { }

/** ROOT MODULE **/

@Component({ selector: 'app-root', template: '<foo></foo> <bar></bar>' })
export class AppComponent { }

@NgModule({
  imports: [
    BrowserModule,
    FooModule, // <--- Switching these
    BarModule, // <--- Switching these
  ],
  declarations: [AppComponent],
  bootstrap: [AppComponent],
})
export class AppModule { }

In the given example only I am foo! printed. If I change FooModule with BarModule in imports then only I am bar! shows up.. What the .....? How should I order to print both of them? :sob:

Btw I'm using angular-cli.

abner commented 7 years ago

The solution proposed in #1644 worked for me:

...
mangle: { screw_ie8 : true, keep_fnames: true }, //prod
thecritic commented 7 years ago

@ediri Using an RC in production is bad, there's no question about that (we are not doing that btw). The point that I was trying to make is that most people actually don't expect there to be such significant changes at this stage and migrating to the new modules was obviously inevitable. It basically threw everything apart for us (the app is quite large at this point), refactoring takes time and sometimes - like in this case - refactoring almost seems impossible.

marcalj commented 7 years ago

Imagine all of this but adding ngUpgrade in the middle :P

We like rock'n'roll! :)

thecritic commented 7 years ago

The (temporary) fix by @hansl works btw, I'm delighted!

[Edit] Actually, not quite sure if that works...

canhamd commented 7 years ago

I has complete success with @sirajc's approach of including all of your declarations in the bootstrap section as well, as long as the component you actually want to bootstrap is the first in the array.

@NgModule({
    imports: [BrowserModule, FormsModule],
    declarations: [AppComponent, HomeComponent, FooterComponent],
    bootstrap: [AppComponent, HomeComponent, FooterComponent]
})

I had some success playing with the order of the components in the declarations section (getting the parent component rendering), but in my case I had two peer child components in a parent component and with reordering I only got one child component or the other rendering.

Note: this issue is only when minifying and/or mangling/uglifying.

IgorMinar commented 7 years ago

Thanks to @hansl, we got to the bottom of this last night.

The code generation uses Function#name in several places, which is not a problem with Ahead of Time (AOT) compilation, but if we are in the JIT mode and the input is minified code then name collisions occur.

Declarations reordering might make the problem go away, but there still might be hidden surprises with that approach since its not really solving the problem.

The only safe fix right now is to configure uglify with mangle: { screw_ie8 : true, keep_fnames: true} or use AOT template compilation.

We are looking into an actual fix for JIT.

TheLarkInn commented 7 years ago

https://github.com/angular/angular-cli/pull/1662

Tying relevant angular-cli workaround for tracking.

IgorMinar commented 7 years ago

update: JIT fix will required bigger changes in the core since it's not really an issue with the core code/design, but with how the classes are transpiled to functions in various tools. We will consider implementing a workaround in the core once more pressing issue are resolved. In the meantime please use keep_fnames option in uglify or use Angular's AOT compilation with ngc.

thelgevold commented 7 years ago

@IgorMinar Won't there currently be an issue relying on AOT compilation when using ngUpgrade? ngUpgrade doesn't utilize the aot code even if it exists.

kylecordes commented 7 years ago

Aside from this issue with ngUpgrade, I suspect this will mostly be a "doesn't matter at all" in the near future. As soon as CLI uses NGC, that will "raise the bar". Once any random person off the Internet can launch a new project in a couple of minutes with CLI and have template pre-compilation running, no one seriously using Angular to to the point they care about production mode, will be able to justify not using NGC. Other tooling will have to catch up very quickly or be abandoned.

marcalj commented 7 years ago

Please, do not forgive ngUpgrade users, Angular team promote ngUpgrade as a way to officially jump on the Angular 2 boat without doing a complete rewrite. And I understand to be used in production too.

There's not enough feature complete UI projects (Material design, ng2-bootstrap, ui-bootstrap) to completely use Angular 2 right now. It's the last requirement I have to fully move towards Angular 2. In the meantime I have to deal with ngUpgrade.

It would be fantastic if we can use NGC with ngUpgrade.

If for instance you decide to use ngUpgrade as a step to help migration but to not using it production, please tell us now to taking that into account.

Thanks a lot.

mii9000 commented 7 years ago

I am facing the same problem and I have been directed to this issue. But I am on Beta 8. Is this issue still applicable in Beta 8?

ericmartinezr commented 7 years ago

@Ibrahim-Islam you are on one of the betas affected by a minification issue that lasted like for 10 betas (beta.1 to ~beta.15 or so). It has nothing to do with this issue though.

aroget commented 7 years ago

+1 Using a Webpack build (not CLI) as @IgorMinar suggested building without minifying works... just like the beta releases a several months ago... I am glad I didnt upgrade our app at work last Friday.....

ofuangka commented 7 years ago

Where do you put mangle: { screw_ie8 : true, keep_fnames: true}? I'm having trouble figuring out how to reconfigure uglify when using angular-cli.

TheLarkInn commented 7 years ago

We have already merged this change in master for the CLI.

kylecordes commented 7 years ago

@TheLarkInn I think @ofuangka was asking: where should this be put today, to have it work, rather than at an unknown future date when a new CLI release ships?

(The best answer is probably: nowhere - instead, use "npm link" to run with the current master CLI instead of the released version.)

ofuangka commented 7 years ago

@kylecordes Thanks! I was under the impression that the solution could be applied through project configuration. Using npm link worked for me.

GaryB432 commented 7 years ago

Whatchu mean "NGC" @kylecordes ?