0

I'm trying to implement a delete epic with a confirmation dialog.

I came up with this approach. It has the advantage of being easy to test.

My question is, is this a good approach, should I worry about adding takeUntil(action$.ofType(MODAL_NO_CLICKED))?

Please let me know if you can think of a better way to implement this.

    const deleteNotification$ = (id, { ajax }) => ajax({ url: `api/delete/{id}` });

    // showYesNo is an action to eventually show a dialog using this approach https://stackoverflow.com/a/35641680/235659
    const showYesNo = payload => ({
        type: SHOW_MODAL,
        modalType: MODAL_TYPE_YES_NO,
        modalProps: { ...payload },
    });

    const deleteNotificationEpic = (action$, store, dependencies) => {
        let uid = dependencies.uid; // dependencies.uid is added here to allow passing the uid during unit test.

        return merge(
            // Show confirmation dialog.
            action$.pipe(
                ofType(NOTIFICATION_DELETE_REQUEST),
                map(action => {
                    uid = shortid.generate();

                    return showYesNo({
                        message: 'NOTIFICATION_DELETE_CONFIRMATION',
                        payload: {
                            notificationId: action.notificationId,
                            uid,
                        },
                    })
                }),
            ),

            // Deletes the notification if the user clicks on Yes
            action$.pipe(
                ofType(MODAL_YES_CLICKED),
                filter(({ payload }) => payload.uid === uid),
                mergeMap(({ payload }) =>
                    deleteNotification$(payload.notificationId, dependencies).pipe(
                        mergeMap(() => of(deleteNotificationSuccess())),
                        catchError(error => of(deleteNotificationSuccess(error))),
                    ),
                ),
            ),
        );
    };

I know I can show the confirmation dialog on the React level and only dispatch the delete action if the user clicks on Yes, but my question is a more general case where I might have some logic (calling the back-end) before deciding to show the confirmation dialog or not.

Anas
  • 5,622
  • 5
  • 39
  • 71

2 Answers2

2

Your solution is generally good. There is a potential for weird bugs since MODAL_YES_CLICKED is always listened for even if the notification isn't displayed, though whether this matters is debatable.

When I need similar patterns I personally set up the listener only as-needed and make sure to have some way to cancel (like MODAL_NO_CLICKED) so I don't leak memory. Putting it more sequentially like this helps me understand the expected flow.

return action$.pipe(
    ofType(NOTIFICATION_DELETE_REQUEST),
    switchMap(action => {
        uid = shortid.generate();

        return action$.pipe(
            ofType(MODAL_YES_CLICKED),
            filter(({ payload }) => payload.uid === uid),
            take(1),
            mergeMap(({ payload }) =>
                deleteNotification$(payload.notificationId, dependencies).pipe(
                    map(() => deleteNotificationSuccess()),
                    catchError(error => of(deleteNotificationSuccess(error))),
                ),
            ),
            takeUntil(action$.pipe(ofType(MODAL_NO_CLICKED))),
            startWith(
                showYesNo({
                    message: 'NOTIFICATION_DELETE_CONFIRMATION',
                    payload: {
                        notificationId: action.notificationId,
                        uid,
                    },
                })
            )
        )
    }),
)

One interesting thing about my approach vs. yours is that mine is a bit more verbose because I need to have takeUntil as well as take(1) (so we don't leak memory).

Unit test:

it('should delete the notification when MODAL_YES_CLICKED is dispatched', () => {
        const uid = 1234;
        shortid.generate.mockImplementation(() => uid);
        const store = null;
        const dependencies = {
            ajax: () => of({}),
            uid,
        };

        const inputValues = {
            a: action.deleteNotificationRequest(12345, uid),
            b: buttonYesClicked({ id: 12345, uid }),
        };

        const expectedValues = {
            a: showYesNo({
                message: 'NOTIFICATION_DELETE_CONFIRMATION',
                payload: {
                    id: 12345,
                    uid,
                },
            }),
            b: showToastSuccessDeleted(),
            c: action.loadNotificationsRequest(false),
        };
        const inputMarble = '   a---b';
        const expectedMarble = '---a---(bc)';

        const ts = new TestScheduler((actual, expected) => {
            expect(actual).toEqual(expected);
        });
        const action$ = new ActionsObservable(ts.createHotObservable(inputMarble, inputValues));
        const outputAction = epic.deleteNotificationEpic(action$, store, dependencies);

        ts.expectObservable(outputAction).toBe(expectedMarble, expectedValues);
        ts.flush();
    });
Anas
  • 5,622
  • 5
  • 39
  • 71
jayphelps
  • 15,276
  • 3
  • 41
  • 54
  • I like this solution better, the only issue is testability, I tried with something like `const action$ = ActionsObservable.of( { type: 'NOTIFICATION_DELETE_REQUEST' }, { type: 'MODAL_YES_CLICKED'} Rx.Scheduler.async, );` but I get some weird behavior in jest, any idea how to test this? Thank you. – Anas May 17 '18 at 15:38
  • Found a way t unit test it using marble testing, added the code, than you. – Anas May 20 '18 at 19:23
0

As comments are restricted in length, I'm posting an answer even though it's not yet one.

I don't think I can given you guidance as the example code is missing implementations so it's not clear what exactly happens. In particular, what is showYesNo and deleteNotification$?


Btw, the unique ID you generate is only done once, when the epic starts up. That seems like it would be a bug since unique IDs are not generally reusable?

const deleteNotificationEpic = (action$, store, dependencies) => {
  const uid = shortid.generate();
jayphelps
  • 15,276
  • 3
  • 41
  • 54
  • Thanks for you answer, yes you're right, uid is not generated each time, I need to fix that. I updated my question with more details, please let me know if you need more information. – Anas May 15 '18 at 22:50