Material-ui: [Snackbar] Reopens when message prop is an element

[iamzhouyi]

This bug happened only when the "message" prop passed as an element(not string)
When the snackbar rendered with same props, it triggers snackbar hide and show..

The reason is the message prop could be an object that never be equal with last one

[alitaheri]

We plan on removing state from these components entirely so this check will never have to happen :grin:

[oliviertassinari]

The state is here to perform the animation. From my perspective, event if we use react-motion to remove the state, the check will still be needed.

[alitaheri]

Hmmm you have a point O.o

[theosherry]

I've reproduced this bug using a FontIcon element as the message for the Snackbar, and having the Snackbar be re-rendered while it's still open:

After a second of opening the Snackbar, the state is changed and the Snackbar re-renders, without it's properties having changed, resulting in the bug:

I've made a PR to add a custom equality tester callback as a property of Snackbar, which addresses this bug.

[oliviertassinari]

I'm wondering if the best fix for this wouldn't be on the user side.
Why not memoizing the message property that's passed down to the <Snackbar /> component?

[theosherry]

Hmm, yeah that does seem like a better solution. I wonder if there should be a warning in the docs that using an element or array type node for the message will require some care in this regard.

[oliviertassinari]

I wonder if there should be a warning in the docs that using an element or array type node for the message will require some care in this regard.

I can't find a way to display such warning :confused:. Any idea?

[iamzhouyi]

if I pass a same key prop on the element, it that possible to determine it' same one?

[theosherry]

@oliviertassinari I just added a bit of a warning in the message documentation: #3416

[theosherry]

@yinickzhou I think the best solution is to actually use the same element if you want the Snackbar to treat it as the same element.

I'm not sure how your code is set up, but an idea is to have the message prop be a function call in which you check if the element should be changed — if it should be changed, make, store, and return the new element; if not, retrieve and return the current message, which should have been stored somewhere.

[oliviertassinari]

@yinickzhou I like this key idea :+1:.

So, what about adding an optional messageId property that would be used instead of the nextProps.message !== this.props.message check when provided? Something like

let messageChanged = false;

if (nextProps.messageId) {
  if (nextProps.messageId !== this.props.messageId ) {
     messageChanged = true;
  }
} else if (nextProps.message !== this.props.message) {
   messageChanged = true;
}

[iamzhouyi]

@thefivetoes I understand why the same element is important, I just think that's not straightforward for the users.
I prefer @oliviertassinari's solution :)

[thefivetoes]

@yinickzhou I think you mean to name check @theosherry?

[iamzhouyi]

yes, sorry about that @thefivetoes

[theosherry]

@yinickzhou I agree it's not all that clear when using Snackbar with an elementmessage what the behaviour will be when it happens to re-render while it's open, which is why I think something has to be changed.

I prefer the solution of having the user ensure the message element doesn't change, because it keeps the Snackbar component more focused; and though the added property might make the component easier to use in this particular case, adding a superfluous property makes the component more difficult to pick up and use in the general case.

[oliviertassinari]

@theosherry @yinickzhou Thanks for the feedbacks. We have merged the PR regarding documenting the behavior. I would say, let's close this issue and see how people react. If this is still an issue, we can reopen it and start looking for better solution.

[rsteckler]

I would prefer the messageId solution proposed by @oliviertassinari

The use case I'm running into is that I want the snackbar to stay on the screen with a 60 second countdown inside of it. I'm using an interval to count down the seconds, storing it in state, then pulling it into the Snackbar:

         <Snackbar
          open={this.state.startNotificationOpen}
          message={'Watch this fancy countdown: ' + this.state.countdown}
        />

Obviously the _message_ is different every time, but I want the snackbar to persist on the screen. Allowing me to pass a messageId would solve my problem, and the more general case of dynamic content in Snackbars.

[oliviertassinari]

@rsteckler That sounds like a valid use case. I'm reopening the issue.

[mbrookes]

IMHO, this is an edge use-case, out of spec, and isn't really what the Snackbar is intended for, so @rsteckler should implement a custom component (using Snackbar as inspiration) rather than adding logic to Snackbar to support it.

[oliviertassinari]

That's definitly an edge use-case.

I think that it would be great to provide a stateless version of the Snackbar so people like @rsteckler can implement their own logic.

[mbrookes]

That's a good point, although I would still be wary of implementing an additional prop to control that behaviour in the current implementation.

What if this functionality was tied to the autoHideDuration prop?

If that is set, the component is then inherently being used statefully, so the current behaviour for re-animating on message change would apply.

If autoHideDuration isn't set, then the component is being controlled statelessly through the open prop, so no animation of the message changes.

This would satisfy both use cases with no additional props, and minimal additional logic.

[rsteckler]

I think the above recommendation works. It basically puts the responsibility for expiring and queuing snackbars in the hands of the developer. I'm wondering, just for clarity, if having a new prop, 'stateless' with a default value of false would help. If stateless == true, then autoHideDuration is ignored.

It just seems more explicit from a documentation point of view, rather than having a side effect attached to autoHideDuration even though they're very closely tied.

[oliviertassinari]

What if this functionality was tied to the autoHideDuration prop?

That's one way to fix it, but I think that we have a better one.
I think that it's the right moment to follow https://github.com/callemall/material-ui/issues/2784, and split the actual Snackbar component into two separate one.

We could create a new Snackbar.stateless.js component as simple as possible. And make the Snackbar.js use it internally.
People may not need the Snackbar.js component, that's fine. For instance, I won't use it, I could move the message show / hide logic into my redux's reducers and use the Snackbar.stateless.js component.

@mbrookes @rsteckler What do you think?

[mbrookes]

I think #2784 would have Snackbar.js be stateless by default, and add a stateful version that wraps it. Not sure how much of ReactStated we'd get to use. @alitaheri - would this be a good place to start implementing that?

[alitaheri]

@mbrookes Since Snackbar doesn't have imperative methods, it's a good place for it :+1: I'll try to make a demo for it.

[juneidysoo]

For what it's worth, I found a workaround by having the message in an array. The source is using === checks which will check for array reference. As long as you are using the same array reference, no reopens will happen.

[azaslavsky]

@juneidysoo do you mind providing an example using the array formulation? I still can't quite get it to work.

[azaslavsky]

@oliviertassinari, I'm a little confused as to how I would memoize @theosherry's example in this comment. Do you mind giving me a short snippet describing how the FontIcon child component in that example could be memoized to pass the this.props.message === nextProps.message check mentioned at the start of this thread?

[juneidysoo]

@azaslavsky My solution is almost the same as @oliviertassinari memoization.
Having the elements (or your message) in an array and keep using the same array and pass it to Snackbar on the proceeding render cycle, you will pass the check message check as equal.

Unfortunately I cannot easily provide you any example code because I have since moved on to other UI library.

P.S. why are you still using this old version? it's 2 years old lol

[Angelk90]

@rsteckler :
Look: https://codesandbox.io/s/material-demo-6zqvp