no-unsafe-first captures safe usage of first

[jrista]

I have encountered a number of use cases where the rxjs/no-unsafe-first rule seems to catch safe usage of first within effects. Whenever an inner stream from a switch|exhaust|concat|merge|flatMap is piped and first is used within that nested pipe, the usage should be save. Not only that, in some cases it is necessary to ensure that the inner streams complete in an appropriate manner, when completion is required. Without usage of first or take within nested streams, particularly with concatMap, mergeMap or flatMap, some inner streams that do not complete can either hang up the queue (concatMap) indefinitely, or result in leaking inner streams (merge|flatMap).

A quick example of some code that interacts with FireStore via AngularFirestore. When a save event is dispatched, one effect either calls set or update, which dispatches another action that allows another effect to wait for either valueChanges or stateChanges to feed the changed data back into the app. The second effect correlates the saved entity when it comes back through, and dispatches a final action representing successful save:

  correlateSavedEntity$ = createEffect(() =>
    this.actions$.pipe(
      ofType(entitySaving),
      mergeMap(({ entity }) =>
        this.actions$.pipe(
          ofType(entityAdded, entityModified),
          map(({ entities }) =>
            entities.find(changed => changed.key === entity.key),
          ),
          first((changed?: Entity): changed is Entity => !!changed),
          map(changed => entitySavedSuccessfully({ entity: changed })),
        ),
      ),
    ),
  );

The inner usage of first here is indeed safe, and has been verified as so (I've just tested several use cases, none of which caused completion of the effect, only of observables nested within mergeMap, flatMap, or concatMap). When a save occurs and the updated entity data is fed back into the app, the inner stream completes, because of the first filter, after it dispatches the success action. Without first, or take, and instead if filter is used on the inner stream, the mergeMap here will leak the inner streams, and every time a save action comes through, all prior inner streams plus an additional one will all handle the entityAdded and entityModified streams. This continues indefinitely, or until the web app is reloaded.

Unsafe usage of first would primarily be within the OUTER stream, not inner streams. So if I used first or take anywhere at the same level as the mergeMap operator is used, that would indeed complete the effect, and would be an unsafe usage. Right now, it appears as though the rule is catching ANY and ALL usage of first and considering it "unsafe", when in fact there are indeed safe use cases for first, take, etc. within effects. Not only that, in some use cases, their usage is essential!

[jrista]

Example of the issue:

Usage should only be called unsafe if it is indeed unsafe. By globally denying use of first within effects (which is what is happening now), it can hamper the ability of developers to implement the most effective code that will solve the problems at hand.

[cartant]

Should be fixed in 4.0.3.