gor181 / react-notification-system-redux

Redux wrapper for react-notification-system
MIT License
324 stars 59 forks source link

Unnecessary rerender/readding for existing messages #17

Closed LKay closed 7 years ago

LKay commented 8 years ago

In this fragment you remove all notifications that are no longer in the state, which is totally fine.

    // Get all active notifications from react-notification-system
    /// and remove all where uid is not found in the reducer
    (this.system().state.notifications || []).forEach(notification => {
      if (notificationIds.indexOf(notification.uid) < 0) {
        this.system().removeNotification(notification.uid);
      }
    });

But here you are looping for each notifications in the redux state even if some are already displayed. You should filter the redux notifications against notification that are already displayed and add only new ones.

    notifications.forEach(notification => {
      this.system().addNotification({
        ...notification,
        onRemove: () => {
          this.context.store.dispatch(actions.hide(notification.uid));
          notification.onRemove && notification.onRemove();
        }
      });
    });
gor181 commented 8 years ago

Hey @LKay ,

Good spot. Will check on how to solve this soon.

gor181 commented 7 years ago

Hey @LKay ,

I think this was done on purpose considering react-notification-system is ignoring notifications with same id anyways so we do not need to filter here.

uid

Overrides the internal uid. Useful if you are managing your notifications id. Notifications with same uid won't be displayed.

Let me know if you are seeing duplicates being added which shouldn't be the case.

gor181 commented 7 years ago

Hey @LKay

I'm closing this one as I have added tests which are confirming that no additional notifications are being added.

thanks!