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

Custom decorators are stripped when using the AoT build flag #2799

Closed
MikeRyanDev opened this issue Oct 20, 2016 · 39 comments
Closed

Custom decorators are stripped when using the AoT build flag #2799

MikeRyanDev opened this issue Oct 20, 2016 · 39 comments
Assignees
Labels
P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent

Comments

@MikeRyanDev
Copy link
Member

OS?

Mac OSX Sierra

Versions.

angular-cli: 1.0.0-beta.17
node: 6.2.1
os: darwin x64

Repro steps.

  1. Clone @ngrx/example-app
  2. Compile the project using the --aot flag
  3. Check the compiled copy of either effect service
  4. Note that the @Effect() decorator from @ngrx/effects has been stripped from the classes

The log given by the failure.

No errors are thrown

@celliott181
Copy link
Contributor

celliott181 commented Oct 20, 2016

I'd like to see this resolved. AoT is great, but custom decorators will be important in the future as more and more third party modules take advantage of the stable API, unlike the current crop out there which tend to be developed on the RCs.

The problem is this needs to be fixed over in Angular 2, not here. @clydin pointed out this issue specifically is in the webpack loader, however there are also issues with decorators in the compiler.
The compiler itself takes a static list of decorators and those are the only decorators that can be AoT compiled.

@clydin
Copy link
Member

clydin commented Oct 20, 2016

The custom webpack loader is what is doing the actual decorator removal: https://github.com/angular/angular-cli/blob/master/packages/webpack/src/loader.ts#L18

@tomwanzek
Copy link

@MikeRyan52 thanks for raising this (and all the hard work on ngrx + effects to begin with).

I agree that is critical to be resolved. I guess the main reason, I have not hit it directly, yet, is that beta.17 with aot and lazy-loading was the initial constraint hitting.

I know, everyone on the angular-cli team is working hard, but the resolution of this issue should be most welcome.

@SethDavenport
Copy link

We have the same thing with @select in the ng2-redux world. If you need more info or help let me know.

MikeRyanDev added a commit to MikeRyanDev/angular-cli that referenced this issue Oct 25, 2016
Previous behavior was to remove all decorators from a source file when using the
@ngtools/webpack loader. This had the side effect of removing any third party
decorators, potentially breaking apps silently when using the aot produciton flag.
This skips the decorator removal process until a better way can be determined
to remove Angular-specific decorators.

Closes angular#2799
@kylecordes
Copy link

I think I saw this same failure mode in an example program that made use of a simple custom decorator. Unfortunately that code is long gone, I ascribed it to programmer error at the time - but once this issue is fixed I will certainly try again.

@filipesilva filipesilva added the P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent label Nov 4, 2016
@rthewhite
Copy link

rthewhite commented Nov 11, 2016

I see the exact same behavior in my application. I have a class with some method decorators:

import { Injectable } from '@angular/core';
import { Observable } from 'rxjs';

import { AuthService, AttributesResponse} from 'api-services';
import { ObservableAction } from 'andux';

@Injectable()
export class UserActions {
  constructor(
    private service: AuthService
  ) {}

  @ObservableAction()
  loadAttributes(): Observable<AttributesResponse> {
    return this.service.loadAttributes();
  }
  @ObservableAction()
  loadPermissions(): Observable<any> {
    return this.service.loadPermissions();
  }
}

When building the application with the AOT option, the decorator is being stripped out by the CLI but the build is a success. Then i run into runtime issues because of the decorator not being applied. The decorators are then also dropped because they are unused.

Dropping unused function ObservableAction [../~/andux/src/actions/observable-action.ts:3,0]
Dropping unused function PaginatableActions [../~/andux/src/actions/paginatable-actions.ts:1,0]
Dropping unused function SortableActions [../~/andux/src/actions/sortable-actions.ts:1,0]
Dropping unused function CrudReducer [../~/andux/src/reducers/crud-reducer.ts:5,0]
Dropping unused function PaginatableReducer [../~/andux/src/reducers/paginatable-reducer.ts:4,0]
Dropping unused function SortableReducer [../~/andux/src/reducers/sortable-reducer.ts:6,0]

We will have to wait for a proper fix as described here: #2881

@hansl hansl modified the milestone: RC1 Nov 11, 2016
@SethDavenport
Copy link

So I understand the tradeoff between tree shaking and allowing custom decorators. However my feeling is that if I have to choose between an optimization (tree shaking) vs. correctness in the code output (not stripping all decorators) I would be inclined to choose correctness first, while I work on re-enabling the optimization afterwards.

This is putting a few people in a fairly awkward position...

@hansl
Copy link
Contributor

hansl commented Nov 11, 2016

Sorry, I haven't commented on this issue and should have.

We will support custom decorators, this is a real issue, but the solution isn't as easy as it seems. There are a lot of performance implications, and build time is an important factor.

The current solution we're considering: dev build will not remove any decorators and as such will not tree shake properly. That's okay, it's dev. Prod builds will only remove decorators that are marked as annotations using tsickle. The prod builds will be slower (as slow as today), while the dev builds (where RTT is much more important) will be larger.

Again, this is what's being considered. The final solution might not be the same. For the time being I'll keep the code as is, but switching to tsickle will happen real soon.

@SethDavenport
Copy link

ok thanks @hansl

@xaralis
Copy link

xaralis commented Nov 18, 2016

This issue should be top priority as it makes larger apps (which usually have some custom decorators in place) impossible to compile ahead of time. And the larger the app is (the more components it has), the longer time the JIT takes. This results in terrible performance and makes angular2 quite problematic to be used in production. I would really appreciate any short-term fix. While tree shaking is nice (and I understand the reasons for decorator stripping), I would gladly accept not having it for some time if that means I am able to do AoT at the moment.

@drager
Copy link

drager commented Nov 18, 2016

I couldn't agree more with @xaralis. We have a large application that actually uses a lot of custom decorators. We also have terrible performance using the JIT and are in the real need of AoT but since the decorators are stripped away we cannot use it... I don't get how this issue only can have priority 2, it should definitely have higher priority.

@Polvista
Copy link

Polvista commented Nov 23, 2016

While we're waiting for fix, I've added temporary version of @ngtools/webpack that doesn't touch decorators: https://github.com/Polvista/ngtools-webpack-keep-decorators.

Usage is pretty easy:
npm install --save ngtools-webpack-keep-decorators
require('@ngtools/webpack') => require('ngtools-webpack-keep-decorators')
loaders: ['@ngtools/webpack'] => loaders: ['ngtools-webpack-keep-decorators']

@sithwin
Copy link

sithwin commented Nov 24, 2016

How do I change because there is no webpack config in cli?
Pls help!

@Polvista
Copy link

@sithwin it won't help if you use angular-cli to perform AOT compilation. However, decorators will work if you compile with webpack + ngtools-webpack-keep-decorators. It's just drop in replacement for @ngtools/webpack.

@petercn
Copy link

petercn commented Dec 8, 2016

Any updates on this would be very much appreciated, I'll try @maxime1992 fork in the meantime and/or try ngc to do my AoT builds instead. Thanks

@maxime1992
Copy link
Contributor

@petercn I did try to rebase from the original repo and things got broken.
If you have some trouble to use it too, you should git checkout the (only) commit I've made.

There is an update tho :
image

@hansl is working on it and we'll probably have a fix in next release ! :)

@petercn
Copy link

petercn commented Dec 8, 2016

Nice, thanks for the update @maxime1992

@nirgn975
Copy link

There is a new version of the cli (beta.23), anyone knows if the fix for this is in there?

@Splaktar
Copy link
Member

Splaktar commented Dec 18, 2016

@nirgn975 yes, a temporary fix via PR #3583 is in while they implement a better permanent solution.

@nirgn975
Copy link

@Splaktar Great, thanks!
Do you know why beta.23 is not publish to npm yet?

@Splaktar
Copy link
Member

@nirgn975 see #3618.

@SethDavenport
Copy link

Just tried again with beta.24 and this appears to be fixed?

@clydin
Copy link
Member

clydin commented Dec 27, 2016

Temporary fix. All decorators are kept but file sizes are now larger

@SethDavenport
Copy link

ok cool, thanks.

@Brocco
Copy link
Contributor

Brocco commented Jan 22, 2017

Decorators are no longer being stripped.

@Brocco Brocco closed this as completed Jan 22, 2017
@KKrisu
Copy link

KKrisu commented Mar 30, 2017

Aren't they stripped again? I'm using custom decorator and it's being stripped on latest version of @ngtools/webpack
Can anyone confirm what's the current approach for custom decorators?
cc @Brocco

@maxime1992
Copy link
Contributor

@KKrisu this is weird. Which version of cli / angular are you using ?

I don't have that problem (and I'm using ngrx/effects, custom decorators were stripped before but not anymore).

@xaralis
Copy link

xaralis commented Mar 31, 2017

@KKrisu I suggest using https://github.com/shlomiassaf/ngc-webpack instead of @ngtools/webpack, it seems way much more mature/stable (at least in present).

@KKrisu
Copy link

KKrisu commented Mar 31, 2017

Hey, @maxime1992 I'm on @ngtools/webpack@1.3.0
I have totally custom decorator built on our own. So the decorator body looks like this:

export function Embeddable(key: string) {
  return function(componentClass: Function) {
    // our custom code
  }
}

then in components

import { Embeddable } from '...';
@Embeddable('link')
@Component({
...

@xaralis thank you, I will consider this alternative if we have more problems, currently, this is the only one with the decorator.

@Alekcei
Copy link

Alekcei commented Jul 3, 2017

Angular cli removes standart decorators. That does't worked at same times AOT and Jit.
How to disable deletion standart decorators?

@fkolar
Copy link

fkolar commented Jul 15, 2017

@Brocco Just wonder when you say decorators are not striped, do you have fix number? It seems it stills removes them.
The standard angular ones. , I just need to keep @input(), @output(), nothing else.

I see in the code (ngtools):

 if (!plugin.skipCodeGeneration) {
                return Promise.resolve()
                    .then(() => _removeDecorators(refactor))
                    .then(() => _replaceBootstrap(plugin, refactor));
            }

Is this is the solution ?

Thank you,
Frank

@DominicBoettger
Copy link

Any news about this? As soon as i compile it with --prod my decorators are removed.

Can i keep them?

@Alekcei
Copy link

Alekcei commented Dec 2, 2017

Even standard decorators are removed.
ng build --prod
Please remove the deletion

@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 Sep 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent
Projects
None yet
Development

Successfully merging a pull request may close this issue.