closure-demo icon indicating copy to clipboard operation
closure-demo copied to clipboard

Compiler brakes ngrx effects

Open vforv opened this issue 8 years ago • 4 comments

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...

vforv avatar Nov 18 '17 18:11 vforv

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.

alexeagle avatar Jan 04 '18 02:01 alexeagle

@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

MikeRyanDev avatar Jan 04 '18 02:01 MikeRyanDev

There are two things:

  • a smoke test that it's possible to use ngrx with closure compiler
  • exhaustive testing that no ngrx feature or use case is broken by closure compiler

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 [email protected] 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 .

alexeagle avatar Jan 04 '18 02:01 alexeagle

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).

alex-okrushko avatar Jan 04 '18 19:01 alex-okrushko