angular / closure-demo

MIT License
114 stars 27 forks source link

Compiler brakes ngrx effects #29

Open vforv opened 6 years ago

vforv commented 6 years ago

Hello @alexeagle

After I add ngrx store it dosn't work on client side, could you check my repo:

https://github.com/vforv/closure-compiler

To start just type:

npm i
npm start

it will build server side with webpack and client with closure...

I get this error:

Uncaught TypeError: this.T is not a function at vb (client.js:28) at client.js:511 at Array.map () at Cr (client.js:511) at b.Dr [as ga] (client.js:512) at b.xl (client.js:58) at b.g (client.js:57) at b.next (client.js:7) at b.next (client.js:12) at b.Fj (client.js:43)

It looks like it brakes EffectsModule... When I remove EffectsModule it works...

alexeagle commented 6 years ago

Maybe @MikeRyanDev would be interested to add some closure compiler compatibility check? We use it at Google so maybe aokrushko can confirm that this works with closure compiler internally.

MikeRyanDev commented 6 years ago

@alexeagle Definitely interested. I know we break it for Closure users semi-regularly but I don't know enough about Closure to add a useful compatibility test suite.

cc @robwormald

alexeagle commented 6 years ago

There are two things:

I don't know of anywhere that we do the second one today. In theory it's possible to re-run your whole unit test suite after closure-compiling the app. The first one is pretty easy to do, but I doubt it would catch issues like this. Should discuss with Rob, maybe we can get enough usage internally at Google that a pre-commit check against all the tests would be sufficient.

On Wed, Jan 3, 2018 at 6:23 PM Mike Ryan notifications@github.com wrote:

@alexeagle https://github.com/alexeagle Definitely interested. I know we break it for Closure users semi-regularly but I don't know enough about Closure to add a useful compatibility test suite.

cc @robwormald https://github.com/robwormald

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/angular/closure-demo/issues/29#issuecomment-355182809, or mute the thread https://github.com/notifications/unsubscribe-auth/AAC5IxxjzufWL3MrYq7xpCV3VSuewFpGks5tHDY0gaJpZM4QjGu3 .

alex-okrushko commented 6 years ago

Hey all,

So the closure strips properties from effects, because "they are not used".

To force them to be there you can declare the interface. That's the best workaround for the moment. I'm also working to find the combination of closure flags that leaves @Effects properties there, yet does property renaming and collapsing.

so the workaround:

export declare interface LoadUserEffectsProps {
   userAcc$: Observable<Action>;
}

@Injectable()
export class LoadUserEffects implements LoadUserEffectsProps {
    constructor(private actions$: Actions, private userService: UserService) {
    }

    @Effect() userAcc$: Observable<Action> = this.actions$
        .ofType(USER_ACTION)
     ...

I was looking into it in December but I was on "vacation" since mid-December and am still on vacation until next week. (vacation with 3 toddlers :) so rarely have time to get to laptop).