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

Effect tests failed after upgrading from 4.1.1 to 5.0.0 #739

Closed
dereklin opened this issue Jan 23, 2018 · 6 comments
Closed

Effect tests failed after upgrading from 4.1.1 to 5.0.0 #739

dereklin opened this issue Jan 23, 2018 · 6 comments

Comments

@dereklin
Copy link

I'm submitting a...


[x ] Regression (a behavior that used to work and stopped working in a new release)
[ ] Bug report  
[ ] Feature request
[ ] Documentation issue or request

What is the current behavior?

Effect tests are failing after the upgrade

Expected behavior:

Effect tests should continue to work since no breaking changes were documented

Minimal reproduction of the problem with instructions:

Clone this repo https://github.com/dereklin/ngrx-store-effects-app/tree/ngrx4-1-1
Make sure to switch to branch ngrx4-1-1
npm install
npm run test
See that all tests pass

Then switch to master: https://github.com/dereklin/ngrx-store-effects-app
Master uses ngrx 5.0.0
npm install
npm run test
See these error messages:

  ToppingsEffects
    loadToppings$
      × should return a collection from LoadToppingsSuccess
        Expected $[0].frame = 0 to equal 10.
        Expected $[0].notification.kind = 'C' to equal 'N'.
        Expected $[0].notification.value = undefined to equal LoadToppingsSuccess({ payload: [ Object({ id: 1, name: 'onion' }), Object({ id: 2, name: 'mushroom' }), Object({ id: 3, name: 'basil' }) ], type: '[Products] Load Toppings Success' }).
        Expected $[0].notification.hasValue = false to equal true.
            at compare (karma.entry.js:79364:33)
            at UserContext.<anonymous> (karma.entry.js:119007:43)
            at ZoneDelegate../node_modules/zone.js/dist/zone.js.ZoneDelegate.invoke (karma.entry.js:115810:26)
            at ProxyZoneSpec../node_modules/zone.js/dist/proxy.js.ProxyZoneSpec.onInvoke (karma.entry.js:115316:39)
            at ZoneDelegate../node_modules/zone.js/dist/zone.js.ZoneDelegate.invoke (karma.entry.js:115809:32)
            at Zone../node_modules/zone.js/dist/zone.js.Zone.run (karma.entry.js:115560:43)
            at UserContext.<anonymous> (karma.entry.js:115014:34)
            at ZoneQueueRunner../node_modules/zone.js/dist/jasmine-patch.js.jasmine.QueueRunner.ZoneQueueRunner.execute (karma.entry.js:115042:42)
            at ZoneQueueRunner../node_modules/zone.js/dist/jasmine-patch.js.jasmine.QueueRunner.ZoneQueueRunner.execute (karma.entry.js:115042:42)
            at ZoneQueueRunner../node_modules/zone.js/dist/jasmine-patch.js.jasmine.QueueRunner.ZoneQueueRunner.execute (karma.entry.js:115042:42)
            at ZoneDelegate../node_modules/zone.js/dist/zone.js.ZoneDelegate.invokeTask (karma.entry.js:115843:31)
            at Zone../node_modules/zone.js/dist/zone.js.Zone.runTask (karma.entry.js:115610:47)
            at drainMicroTaskQueue (karma.entry.js:116017:35)
            at <anonymous>

Version of affected browser(s),operating system(s), npm, node and ngrx:

Other information:

@fulls1z3
Copy link

fulls1z3 commented Jan 24, 2018

@dereklin I had a similar issue with my code (ng-seed/universal) with i18n.effects. While debugging my own code line-by-line and having a look at the commits on @ngrx/store and @ngrx/effects, I found out that the operator ofType should be piped.

In your code at https://github.com/dereklin/ngrx-store-effects-app/blob/master/src/products/store/effects/pizzas.effect.ts#L17, the following change should work:

Instead of

  @Effect()
  loadPizzas$ = this.actions$.ofType(pizzaActions.LOAD_PIZZAS).pipe(
    switchMap(() => {
      return this.pizzaService.getPizzas().pipe(
        map(pizzas => {
          return new pizzaActions.LoadPizzasSuccess(pizzas);
        }),
        catchError(error => {
          return of(new pizzaActions.LoadPizzasFail(error));
        })
      );
    })
  );

you may try this:

  @Effect()
  loadPizzas$ = this.actions$.pipe(
    ofType(pizzaActions.LOAD_PIZZAS),
    switchMap(() => {
      return this.pizzaService.getPizzas().pipe(
        map(pizzas => {
          return new pizzaActions.LoadPizzasSuccess(pizzas);
        }),
        catchError(error => {
          return of(new pizzaActions.LoadPizzasFail(error));
        })
      );
    })
  );

Let us know if that works for you my friend.

@dereklin
Copy link
Author

@fulls1z3 Yes. Thanks. I was troubleshooting this and figured this out as well. Looking at the example-app, the ofType is inside the pipe.

This breaking change should have been documented.

@fulls1z3
Copy link

@dereklin I agree, that should be documented, but as being a library author I can honestly say it's very possible to miss that small detail 😄

I think it's best to send a PR to ngrx team, so they will take care of it 👍

@brandonroberts
Copy link
Member

@dereklin this wasn't intended to be a breaking change. We'll take a look and get a fix in.

@seritools
Copy link

seritools commented Jan 25, 2018

This also broke the API documentation sample over here:

Argument of type '(source$: Actions<Action>) => Actions<Action>' is not assignable to parameter of type 'UnaryFunction<Observable<Action>, Observable<Action>>'. Types of parameters 'source$' and 'source' are incompatible. Type 'Observable<Action>' is not assignable to type 'Actions<Action>'. Property 'ofType' is missing in type 'Observable<Action>'.

Reverting to 4.1.1 for now and hoping for a quick 5.0.1 :)

EDIT: Nevermind, this is just a problem with the tsc strict mode: #753

@MikeRyanDev
Copy link
Member

Fix released in 5.0.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants