fkhadra / react-toastify

React notification made easy 🚀 !
https://fkhadra.github.io/react-toastify/introduction
MIT License
12.33k stars 676 forks source link

Uncaught TypeError: Cyclical Object Value with useNotificationCenter and local storage #969

Closed chip-davis closed 1 year ago

chip-davis commented 1 year ago

Do you want to request a feature or report a bug? A bug, probably on my end.

What is the current behavior? While attempting to implement local storage as seen in #826, I am running into an error with setting the local storage only when I am marking all notifications as read. Single updates on read and delete work fine but mark as all read generates this error. Below is the relevant code.

const {
    notifications,
    clear,
    markAllAsRead,
    markAsRead,
    remove,
    unreadCount,
  } = useNotificationCenter({
    data: JSON.parse(localStorage.getItem("notifications")),
  });

useEffect(() => {
    if (notifications.length !== 0) {
      localStorage.setItem("notifications", JSON.stringify(notifications));
    }
  }, [notifications]);

  const clearNotificationCenter = () => {
    clear();
    localStorage.removeItem("notifications");
  };

<NotificationFooter>
          <NotificationButton onClick={clearNotificationCenter}>
            Clear All
          </NotificationButton>
          <NotificationButton onClick={markAllAsRead}>
            Mark All as read
          </NotificationButton>
</NotificationFooter>

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem. Your bug will get fixed much faster if we can run your code and it doesn't have dependencies other than React. Paste the link to your CodeSandbox (https://codesandbox.io/s/new) example below:

I am not quite sure how to replicate it. The sandbox in #826 works perfectly fine.

What is the expected behavior?

Mark all as read should not cause in error.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React? React 18.15.0, Windows 11, Firefox & Chrome

BranndonWork commented 1 year ago

Hi there, I encountered the same issue with local storage you're dealing with and your ticket pointed me in the right direction to solve it. I believe I can return the favor by assisting you as well.

Here is a working solution that doesn't require any custom 'clear all' methods:

import { useEffect, useState } from "react";
import { useNotificationCenter } from "react-toastify/addons/use-notification-center";
import Utils from "../utils";

const useNotifications = () => {
  const [initialRender, setInitialRender] = useState(true);
  const { notifications, clear, markAllAsRead, markAsRead, remove, unreadCount } = useNotificationCenter({
    data: Utils.localStorage.get("notifications", []),
  });

  useEffect(() => {
    if (!initialRender) {
      Utils.localStorage.set("notifications", notifications);
    }
  }, [notifications, initialRender]);

  useEffect(() => {
    setInitialRender(false);
  }, []);

  return { notifications, clear, markAllAsRead, markAsRead, remove, unreadCount };
};

export default useNotifications;

Adapted to work with native local storage, it would look like this:

import { useEffect, useState } from "react";
import { useNotificationCenter } from "react-toastify/addons/use-notification-center";

const useNotifications = () => {
  const [initialRender, setInitialRender] = useState(true);
  const initialNotifications = JSON.parse(localStorage.getItem("notifications")) || [];

  const { notifications, clear, markAllAsRead, markAsRead, remove, unreadCount } = useNotificationCenter({
    data: initialNotifications,
  });

  useEffect(() => {
    if (!initialRender) {
      localStorage.setItem("notifications", JSON.stringify(notifications));
    }
  }, [notifications, initialRender]);

  useEffect(() => {
    setInitialRender(false);
  }, []);

  return { notifications, clear, markAllAsRead, markAsRead, remove, unreadCount };
};

export default useNotifications;
chip-davis commented 1 year ago

@BranndonWork Wow awesome thanks so much! I'll implement this and see if it works for me.

fkhadra commented 1 year ago

Thanks @BranndonWork for sharing your solution!

chip-davis commented 11 months ago

Anyone else who comes across this I was also able to use the flatted package to resolve the issue 😀

bogdan-ivan commented 7 months ago

@chip-davis how did you solve the issue ?

chip-davis commented 7 months ago

@chip-davis how did you solve the issue ?

Using the flatted library, import the necessary functions (probably just need stringify) you need and use them when setting your storage.


import { parse, stringify, toJSON, fromJSON } from “flatted";

sessionStorage.setItem("notifications",
stringify(notifications));
bogdan-ivan commented 7 months ago

@chip-davis But this causes the notification item in localStorage to have 8000+ entries when you press "Mark all as read": image And by using new Blob([localStorage.getItem('your-key')]).size to check the size, you can see that 0.825 megabytes are used no matter how many notifications you had before pressing "Mark all as read". Is there no better way that uses less space ?

chip-davis commented 7 months ago

@chip-davis

But this causes the notification item in localStorage to have 8000+ entries when you press "Mark all as read":

image

And by using new Blob([localStorage.getItem('your-key')]).size to check the size, you can see that 0.825 megabytes are used no matter how many notifications you had before pressing "Mark all as read".

Is there no better way that uses less space ?

Been awhile since I was working on this codebase I had the issue in and don't remember that being a problem for me :/

Maybe try the other solution posted here?