feat: Make hooks trigger rerender. #99
Conversation
|
@greg2012201 @andreizanik wow, thx for taking actions, looking forward to this being in production! |
|
Hello! Cookie storage is not state manager like redux or mobx. And update cookie value shouldn't trigger a rerender of the component. Standard cookie libraries are not reactive I think, if we merge this PR we will break the expected behavior of working with cookies But I understand that it can be useful for some developers. May be it'll be better to implement new hooks (something like getCookiesReactive, setCookiesReactive) and the old exported functions and hooks to remain unchanged? |
|
@andreizanik @greg2012201 I understand what you are saying, however, since package has different set of functions that are having read on cookie store, which some of them are just normal functions (such as getCookie, setCookie etc) and some of them are hooks (useGetCookies, useSetCookie etc.), I personally would expect second set (namely hooks) be reactive since hooks in react basically functions that I respect your concern about such change that may cause breaking changes in the code, so I think it either can be a separate hook in the package or just application of fix to all hooks in the library and making it a part of some major release with announcement of migration guide and major change description would be the way to go (for me personally), since I still expect those hooks work the way I just described above (due to developer experience mismatch). Looking forward to hearing your opinion about this concern! Thank you for finding time to review my github issue and respond to it, very appreciate that! |
|
I think we can add reactive hooks for setter functions only. Regarding @yasinitskyi’s point about hooks: I understand both of your perspectives, and I’m proposing the following changes:
What do you think? |
|
@greg2012201 thank you for your sharing on this matter, and thank you for your proposal. I think either way how you want to go about it, either tweak previous hooks or add new set of hooks to unlock/expand needed behavior, will work. As long as we can get the functionality, I think we are good. |
|
@greg2012201 Small detail, it is important I guess not only get re-rendering on cookie set but also on cookie get, if cookies are set in one component but are read in another... Reactivity from set function would be helpful in terms of the same scope, but it is not always the case, you would want reactivity on read as well, like from my example in #97 |
@yasinitskyi I agree with you. If we decide to add reactivity, we must add it to all hooks And setting/delete/update cookies in one component should force re-render to other components that use hooks useGetCookie('name_of_the_changed_cookie'), useHasCookie('name_of_the_changed_cookie'), useGetCookies() or useGetCookieReactive('name_of_the_changed_cookie'), useHasCookieReactive('name_of_the_changed_cookie'), useGetCookiesReactive() In my opinion, without this, the update doesn't make sense @greg2012201 what do you think? |
|
I generally agree. If we decide to move forward with this, we have at least two implementation options:
Which approach do we want to go with? P.S. If we decide to proceed, I’ll need approximately a month to ship these changes, as I’m currently a bit busy. |
|
@andreizanik, which approach do we want to pursue? |
|
@greg2012201 I might not be an expert here, but I feel context may not be a good idea since it will lead to whole tree to re-render which is not ideal, event sound more legit perhaps... |
|
In my opinion context will be better. Because it is providing by React and it will be more familiar for developers. We often use different providers if we want to get rerendering after data mutation (For instance TanStack Query and different tools for state managment). I think it's our case. This is my opinion and I am open to discussion |
|
It’s fine by me. Using context for that type of stuff is a well-established pattern in the React ecosystem, and it should work well. |
|
I have completed the implementation of reactive hooks. Here’s what’s included:
I also discovered a potential issue with the For now, the same issue persists in the functions returned from the reactive hooks (getCookies, getCookie). If needed, After merging the current PR I can submit an another to address this. Let me know if you would like me to proceed. The issue: #101 I believe my changes will make As always, I performed manual testing, but I would appreciate it if you could also review and test the code to ensure quality. Looking forward to your feedback! |
|
@greg2012201 That is, we will be able to implement such a behavior when we will synchronize the context only when it is necessary and without delays related to poolingOptions.intervalMs What do you think? |
|
It might be a good idea. I could create a new hook called Since I will be busy for the next 2-3 weeks, I suggest merging the current PR for now. I can then create a new one with this manual refresh feature when I’m available. @andreizanik What do you think? |
|
Got it. |
|
Ok, I am looking forward to your review. |
Hello! 👋🏻 @yasinitskyi @andreizanik
Here is the fix for the issue: #97. I've added a little workaround with using state updates to ensure component re-render when cookies change.
As always, please test the PR on some Next.js project before merge (I might have missed something).
Thanks in advance for the code review.