Simplify KaizenProvider #5426
Simplify KaizenProvider #5426
Conversation
🦋 Changeset detectedLatest commit: 032f7d8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| setDocumentIsAvailable(true) | ||
| } | ||
| }, [documentIsAvailable]) |
There was a problem hiding this comment.
it doesn't need to be a dependency, nothing inside useEffect is referring to it
| @@ -175,26 +175,27 @@ export const NoDuplicatesWithSameId: Story = { | |||
| render: () => { | |||
| const { addToastNotification } = useToastNotification() | |||
|
|
|||
| useEffect(() => { | |||
There was a problem hiding this comment.
this story was failing before I made this change due to the toast was not visible.
Not sure why that was the case but I think using useEffect here might have cause some issue as I suspect addToastNotification is not being wrapped inside a useCallBack so having it inside useEffect dependency will cause it to be called many times
| }, [addToastNotification]) | ||
|
|
||
| return <div>Irrelevant content</div> | ||
| return ( |
There was a problem hiding this comment.
The main issue I'd have with you changing this test is that I think some teams actually use addToastNotification in a useEffect in a similar fashion to this. If it fails here, then it will start throwing errors for consumers too.
I suggest you make a canary and test against performance-review-cycles-ui and see if everything passes or not.
Slack thread of the SSR issue when ToastNotificationProvider was introduced: https://cultureamp.slack.com/archives/C0189KBPM4Y/p1700782380804669
There was a problem hiding this comment.
let me test it out. I reckon all these addToastNotification updateToastNotification removeToastNotification clearToastNotifications functions should be wrapped inside an useCallback tbh as they can be used as dependency in useEffect
There was a problem hiding this comment.
actually I will need to test a canary from 1.67.0 version instead as the latest version has some issue
There was a problem hiding this comment.
read through your comment here about what the [] dependency array which will cause the issue that I am seeing with this story hence trying to fix it.
That is weird though as documentIsAvailable is not even being used inside useEffect - having it in the dependency array will just cause the useEffect to run again after setDocumentIsAvailable(true) has been called
There was a problem hiding this comment.
I always get this error with pnpm storybook even after pnpm reset so unable to run locally and test it
Important: Request PR reviews on Slack
Please reach out to the design system team on Slack in
#prod_design_systemfor PR reviews. GitHub notifications (e.g. from tagging a person) are not actively monitored.Why
We have built a shared component as part of
@cultureamp/conversations-shared-uiwhich makes use ofuseToastNotification. This hook relies onKaizenProviderbeing set up by the consumer and most importantly, the consumer needs to have the exact same version as the requiredpeerdependency that we specify in@cultureamp/conversations-shared-ui. That is not always gonna be the case though and it will be fragile hoping for consumers to always have the same version.Due to that, I am setting up
KaizenProviderin our component alongside with other providers to ensure that we dont have to rely on the consumers and prevent it breaking in production (it will throw error when the context is not found)When I do the above though, it threw this error which Michael encoutered as well so decided to take a look at
KaizenProviderand noticed that it could be improved so thought to try to see if it fixes the issue above and it did.What