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

enqueueSnackbar multiple times ignores maxSnack #38

Closed
Kizmar opened this issue Dec 9, 2018 · 13 comments · Fixed by #189
Closed

enqueueSnackbar multiple times ignores maxSnack #38

Kizmar opened this issue Dec 9, 2018 · 13 comments · Fixed by #189

Comments

@Kizmar
Copy link

Kizmar commented Dec 9, 2018

Expected Behavior

I'm using CustomEvents to determine when to show the snackbar. On the home page I have some temp methods to test using this library.

The provider in App.tsx: <SnackbarProvider maxSnack={3} anchorOrigin={snackbarOrigin}>...

I have a HoC using withSnackbar(Component) with the following event handler:

private handleDidHappen = (event: CustomEvent<IGlobalAlertEvent>) => {
	const detail: IGlobalAlertEvent = event.detail;
	const message: string = `${detail.title} ${detail.message}`;
	const options: OptionsObject = {
		variant: detail.variant,
		autoHideDuration: detail.autoHideMs
	};

	this.props.enqueueSnackbar(message, options);
}

On the home page I have 5 buttons that fire off events that get picked up by that HoC (Test All, Test Info, Test Warning, Test Error, Test Success). The single event buttons work as I would expect when I randomly click them several times. I expected clicking the "Test All" button would show 3, then show the 4th when the first one auto-hides.

Current Behavior

If I click the "All Alerts" button, which instantly fires 4 events, I'm seeing all 4 snackbar items show at the same time, then all disappear at the same time.

Steps to Reproduce

Set the provider max to 3, then make a click event that runs enqueueSnackbar 4+ times in a row with no pause in between.

BTW this template no longer exists?

This should be pretty easy to reproduce, but I can create a codesandbox if needed.

Context

The reason I'm testing this scenario: I was using a custom mobx store to do what this library does, but wanted to try this out. Since this is used when API calls error (among other things), there are times where you might have several events fire in rapid succession.

Your Environment

Project is based on create-react-app using TypeScript. Running on Windows 10, vscode. Everything is current version.

| Tech | Version |

"dependencies": {
	"@material-ui/core": "^3.6.1",
	"@material-ui/icons": "^3.0.1",
	"axios": "^0.18.0",
	"date-fns": "^1.29.0",
	"mobx": "^5.6.0",
	"mobx-react": "^5.4.2",
	"mobx-react-router": "^4.0.5",
	"notistack": "^0.4.0",
	"oidc-client": "^1.5.4",
	"query-string": "^6.2.0",
	"ramda": "^0.26.1",
	"react": "^16.6.3",
	"react-dom": "^16.6.3",
	"react-json-view": "^1.19.1",
	"react-router-dom": "^4.3.1",
	"react-scripts-ts": "3.1.0",
	"validator": "^10.9.0",
	"typescript": "^3.2.1"
},
@Kizmar Kizmar changed the title enqueueSnackbar multiple times is wonky enqueueSnackbar multiple times shows more than maxSnack Dec 9, 2018
@Kizmar Kizmar changed the title enqueueSnackbar multiple times shows more than maxSnack enqueueSnackbar multiple times ignores maxSnack Dec 9, 2018
@iamhosseindhv
Copy link
Owner

@Kizmar Thanks for reporting this. I'll investigate this. It'd be nice to have a sandbox reproduction though.

I'll update sandbox link in issue template to this.

@Kizmar
Copy link
Author

Kizmar commented Dec 22, 2018

Sorry for the delay, been scrambling with the holidays coming up. I'll try to get a sandbox reproduction up here in the next day or two.

@iamhosseindhv
Copy link
Owner

Any update?

@iamhosseindhv
Copy link
Owner

Closed due to inactivity. feel free to reopen anytime.

@babradshaw
Copy link

babradshaw commented Apr 18, 2019

I have the same issue ... mobx store starts with 4 items which are enqueued during the notifier mount ... all 4 are displayed even though maxSnack=1 on the SnackProvider. I thought it would internally queue the notifications when more are enqueued than the max.

If I have persist on I see this in the console:

Reached maxSnack while all enqueued snackbars have 'persist' flag. Notistack will dismiss the oldest snackbar anyway to allow other ones in the queue to be presented.

@iamhosseindhv
Copy link
Owner

I'll reopen and investigate this.

@iamhosseindhv iamhosseindhv reopened this Apr 19, 2019
@MortezaHeydari97
Copy link

MortezaHeydari97 commented May 8, 2019

Hi, I've the same problem. When I try to show a snackbar, at the first time, it will not declare and at the second time it shows two times.

Can you help me how can I fix this?

@iamhosseindhv
Copy link
Owner

iamhosseindhv commented May 8, 2019

@MortezaHeydari97 please share the code you have to enqueue snackbar.

@MortezaHeydari97
Copy link

@MortezaHeydari97 please share the code you have to enqueue snackbar.

I fixed it, so thanks.

@tomzaku
Copy link

tomzaku commented Sep 27, 2019

I have the same issue, My code sandbox here: https://codesandbox.io/s/notistack-redux-example-gyw9u
How can I solve that

@AnaBrade
Copy link

AnaBrade commented Oct 7, 2019

We have the same issue as tomzaku. It seems that all configurations get ignored when adding snackbar while iterating through an array. I tried to fix the issue by setting preventDuplicate totrue and I still get multiple snackbars with the same message. Can be replicated in tomzaku's sandbox by adding preventDuplicate: true.

@simonbos
Copy link
Contributor

A minimal example for this bug: https://codesandbox.io/s/notistack-simple-example-xpff1 (click on the default button). Important lines:

  handleClickWithAction = () => {
    this.props.enqueueSnackbar("Customise this snackbar youself.");
    this.props.enqueueSnackbar("Customise this snackbar youself.");
    this.props.enqueueSnackbar("Customise this snackbar youself.");
    this.props.enqueueSnackbar("Customise this snackbar youself.");
  };

The bug appears when the enqueueSnackbar method is called multiple times in one synchronous method, or before the React tree re-renders. I believe the origin of the bug lies in the following code (from SnackbarProvider.js)

    /**
     * Display snack if there's space for it. Otherwise, immediately begin dismissing the
     * oldest message to start showing the new one.
     */
    handleDisplaySnack = () => {
        const { maxSnack } = this.props;
        const { snacks } = this.state;
        if (snacks.length >= maxSnack) {
            return this.handleDismissOldest();
        }
        return this.processQueue();
    };

    /**
     * Display items (notifications) in the queue if there's space for them.
     */
    processQueue = () => {
        if (this.queue.length > 0) {
            const newOne = this.queue.shift();
            this.setState(({ snacks }) => ({
                snacks: [...snacks, newOne],
            }));
        }
    };

When handleDisplaySnack is called, the snacks array is fetched from the state and checked if the length of the array is greater or equal than maxSnack. If not, the state adds a new snack from the queue.

However, the bug appears because setState is asynchronous: at the beginning of the above handleClickWithAction, the length of const { snacks } = this.state; is 0. At the end, after the four enqueueSnackbar invocations it will be still 0, and it will remain that way untill the setState calls are effectively executed. The relevant React docs:

setState() does not always immediately update the component. It may batch or defer the update until later. This makes reading this.state right after calling setState() a potential pitfall.

In my view, a solution for this error would require to handle snacks.length >= maxSnack in the setState reducer, i.e. something like:

this.setState((prevState) => ({
    if (prevState.snacks.length >= maxSnack)
       return handleDimissOldest(prevState)
    return processQueue(prevState)
}));

Any thoughts @iamhosseindhv ?

@iamhosseindhv
Copy link
Owner

correct @simonbos. The issue originates in asynchronous nature of setState. #15 tried and somewhat managed to solve the issue. If you follow the discussion in that thread you'll find out why it didn't get merged. But I like the general idea in #15 and this issue is on top of my todo list.
Any PR welcome.

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

Successfully merging a pull request may close this issue.

7 participants