Platform: Schematics: Default effect leads to an endless loop

[fmalcher]

The command ng generate feature does create an effects class with the following effect (example):

@Effect()
loadBooks$ = this.actions$.pipe(ofType(BookActionTypes.LoadBooks));

(see https://github.com/ngrx/platform/blob/master/modules/schematics/src/effect/files/__name%40dasherize%40if-flat__/__name%40dasherize__.effects.ts#L24)

If left untouched, this will lead to a loop of death as soon as the first LoadBooks action is being dispatched.

Possible solutions

• Comment out the @Effect() decorator by default
• don't provide a default effect after creation
• set { dispatch: false }

I don't like any of them – but I ran into that the infinite loop problem multiple times now 😆
Will prepare a PR if there is consensus about how we can solve this.

I would be willing to submit a PR to fix this issue

[x] Yes (Assistance is provided if you need help submitting a pull request)
[ ] No

[timdeschryver]

We recently merged PR #1530 which resolves this issue 😅 .
It will become available in the next version.

[brandonroberts]

1530 was released with 7.2.0 👋

[timdeschryver]

The docs aren't reflecting these changes, would it be OK to open up a new issue to add the api flag introduced in the PR?

[fmalcher]

Thanks for the quick reply! However, I'm not sure https://github.com/ngrx/platform/pull/1530 actually fixes this.

With ng g feature foobar --api I actually get a "harmless" effect –
but ng g feature foobar still produces this one:

@Injectable()
export class FoobarEffects {

  @Effect()
  loadFoobars$ = this.actions$.pipe(ofType(FoobarActionTypes.LoadFoobars));

  constructor(private actions$: Actions) {}
}

Version is 7.2.0 installed today 😇

What do you think about completely removing the sample effect if the --api flag is not used?

[brandonroberts]

It's easier to delete the code than to write it from scratch. I think we should at least add a note in the docs that the generated effect must be updated to return a new action.

I don't usually dispatch the action until the effects are wired up but I can see how it causes an issue.

[timdeschryver]

You're right @fmalcher.

We discussed the possible solutions in the issues thread, https://github.com/ngrx/platform/issues/1524#issuecomment-457450857.

With @fmalcher's proposed solutions in mind, should we re-open this issue or leave this as is. If we re-open the issue do we still agree that commenting out the effect is the best solution?

[brandonroberts]

I want to put developers on the path of continuing with minor modifications. I'd recommend we do something similar to the --api and concatMap to an EMPTY observable by default.

[fmalcher]

We discussed the possible solutions in the issues thread, #1524 (comment).

Ah I see! Haven't seen this before -- this would have made THIS whole issue obsolete 🙈 sorry

I'd recommend we do something similar to the --api and concatMap to an EMPTY observable by default.

Sounds good to me! I could live with both ways:

(1) concatMap to EMPTY

@Effect()
loadFoobars$ = this.actions$.pipe(
  ofType(FoobarActionTypes.LoadFoobars),
  concatMap(() => EMPTY) // remove or replace with your own side effect execution
);

(2) comment out

// @Effect()
// loadFoobars$ = this.actions$.pipe(ofType(FoobarActionTypes.LoadFoobars));

---

As I find (1) a sensible solution, should I submit a PR for this?
@brandonroberts @timdeschryver

[fmalcher]

Slightly related:
ng g feature foobar --api injects Actions in the new typed way:

private actions$: Actions<FoobarActions>

while ng g feature foobar still uses the old way where we need to type ofType explicitly:

private actions$: Actions

Am I right assuming that this is not intended and should be changed as well? 😅

[timdeschryver]

I would also prefer option one 😄

The reason why ng g feature foobar doesn't use the new way, is because we don't need to infer the payload of the action. But perhaps it would be better to still type the actions? 🤔 .

[brandonroberts]

It should still be typed when generating a feature because we have an actions union.