Fix setState race conditions

[simonbos]

This pull request fixes #38 and #175. Main rationale of the pull request:

• Using reducer functions to adapt the state to avoid race conditions.
• handleDismissOldest no longer removes closed messages, this should only happen in handleExitedSnack.
• Introduced entered and requestClose boolean variables. With these variables, a snackbar should first have fully entered the window before it can be dismissed. This is needed for e.g. the case maxSnack = 1 and enqueueSnackbar called twice. Without the variables, the open prop of the first snackbar would immediately be set to false, without the snackbar ever being rendered. In this case, the handleExitedSnack function would never be called for that snackbar and it would be always in the view (although hidden due to open = false).
• Added queue: queue.filter(item => item.key !== key), to handleCloseSnack for Persistent snackbar not (always) closing with closeSnackbar(key) #175.

Old codesandbox (#38): https://codesandbox.io/s/notistack-simple-example-xpff1
Fixed codesandbox: https://codesandbox.io/s/notistack-simple-example-yd6gw

Note: a possible 'breaking' change is that enqueueSnackbar no longer returns null as id if it prevented a duplicate. (This is due to the asynchronous setState.)

Any comments are welcome.

[iamhosseindhv]

Well done @simonbos 👏🏼👏🏼 One of the best PRs from the community here.

Changes:

1. Correct me if I'm wrong, but it seemed unnecessary to pass props to the reducers? We can always access them using this.props can't we?
2. User-defined onEntered callbacks get called now. whether it's passed globally through SnackbarProvider or options of enqueueSnackbar.
3. Some refactoring to silent linting rules.

Let me know what you think about the changes I made.

[simonbos]

Thanks for the updates, looks very good in general! Some remarks:

1. For passing the props to the reducers, how I understand it: if we use the prevProps from setState, the props from the previous render (from when you call the setState function) are used. If we use this.props, it could be the updated props. I don't think it matters much in notistack, most of the props seem pretty static. However, I am more inclined to use the prevProps from setState since 1) I think it could be more 'safe' and 2) then the reducers are more pure functions.
   Reference: https://reactjs.org/docs/react-component.html#setstate

2. Your first commit introduced some regressions; the extra commit should fix them.

3. Lastly, before merging this PR, one extra thing has to be cleared up. As of now, when a user calls closeSnackbar(key) immediately after key = enqueueSnackbar, the snack will be in the state open = false and it's handleExitedSnack method will never really remove the snack from the queue. We have two solutions for this:

   • Wait untill the snackbar is entered before closing (i.e. setting requestClose=true)
   • Immediately remove the snackbar without ever showing it, and processing the queue.

I would opt for the second option (e.g for a Loading Snackbar), but we could also support both use cases (e.g. with a shouldHaveEntered boolean). (Note: this boolean could then also be used for items in the queue which are removed.)

[iamhosseindhv]

1. I completely agree with you. But since in our case this.props is very unlikely to be changed, I'd go with less code and passing parameters (i.e. use this.props instead of prevProps). We can revisit this matter in future.

2. Nice catch. That's what happens when you do blind copy-paste.

3. I'd go with the second options as well. and not giving the user that option. At the moment we accept 28 props. I'll do anything in my power not to add a new one to the list 😁.

Haven't tested this but I'd propose:

 handleCloseSnack = (event, reason, key) => {
        // ...
        if (reason === REASONS.CLICKAWAY) return;

        const shouldCloseAll = key === undefined;

        this.setState(({ snacks, queue }) => ({
            snacks: snacks
                .filter(item => (
                    // calling close on a snackbar that hasn't entered the screen yet.
                    // i.e. call closeSnackbar immediately after enqueueSnackbar.
                    (shouldCloseAll || item.key === key) && !item.entered
                ))
                .map(item => (
                    (shouldCloseAll || item.key === key) ? { ...item, open: false } : { ...item }
                )),
            queue: queue.filter(item => item.key !== key), // eslint-disable-line react/no-unused-state
        }));
    };

Lmk what you think. @simonbos

[simonbos]

Ok for 1-3! I agree that no extra props should be added for a feature of which we aren't sure if it would be used 😉 If such a feature would be implemented in the future, I would go for adding a parameter to the function, i.e. something in the style of handleCloseSnack = (event, reason, key, shouldHaveEntered=false) and closeSnackbar = (key, shouldHaveEntered=false). (As such, both use cases could be used at the same time.)

Your code looks good; the only thing that should be added is that if an element is effectively filtered in the filter step, then the queue should also be processed (i.e. an element is removed from the snacks view, thus the first element in the queue should take its place.). If after this, the queue isn't empty, the handleDismissOldest function should also be called. (Note: this is the same process as in the handleExitedSnack function)

Adapted code (unused and unoptimal!!):

handleCloseSnack = (event, reason, key) => {
    // ...
    if (reason === REASONS.CLICKAWAY) return;

    this.setState((state) => {
        // remove all messages if no key specified
        if (key === undefined)
            return {...state, queue: [], snacks: []};

        // delete specific snack from queue
        const newQueue = state.queue.filter(item => item.key !== key); // eslint-disable-line react/no-unused-state

        // delete specific snack if not entered
        if (state.snacks.some((item) => item.key === key && !item.entered)) {
            const newState = this.processQueue({
                ...state,
                snacks: state.snacks.filter(item => item.key !== key),
                queue: newQueue,
            });

            return newState.queue.length === 0
                ? newState
                : this.handleDismissOldest(newState);
        }

        // close specific snack if entered
        return {
            ...state,
            snacks: state.snacks.map(item => (
                (item.key === key) ? { ...item, open: false } : { ...item }
            )),
            queue: newQueue,
        }
    });
}

However, as you can see in the following codesandbox, this code is not optimal (click default): https://codesandbox.io/s/notistack-simple-example-8mlvg The issue appears because the entered state is applied only once the transition is fully over. This issue could be solved by also passing the onEntering callback. Instead, I reconsidered option 1. The result is visible in the next commit and in the following codesandbox: https://codesandbox.io/s/notistack-simple-example-s84i6 As of now, I like this solution much more. I do recognize that having the option to close a snackbar immediately is handy. Therefore, I will create a seperate PR for this feature. It will be along the lines of:

    closeSnackbar = (key, immediately=true) => {
        if (immediately)
            this.handleExitedSnack(null, key)
        else
            this.handleCloseSnack(null, null, key);
    }

If we do it like this, the responsibilities of the functions are nicely divided: handleCloseSnack is for setting open=false, handleExitedSnack is the only function responsible for deleting snacks.

Any last remarks, @iamhosseindhv ?

[iamhosseindhv]

@simonbos

Perfect. I think everyone using notistack, specially myself, appreciate your great work.

Happy to merge?

[simonbos]

@iamhosseindhv It's my pleasure, thank you for the nice package! Merge 👍