Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/remove unneccesary dependency array #1079

Merged

Conversation

paghar
Copy link
Contributor

@paghar paghar commented Oct 20, 2023

Summarize the changes made and the motivation behind them.

Reference related issues using # followed by the issue number.

If there are breaking API changes - like adding or removing props, or changing the structure of the theme - describe them, and provide steps to update existing code.

@vercel
Copy link

vercel bot commented Oct 20, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
flowbite-react ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 26, 2023 10:54am

@paghar paghar force-pushed the fix/remove-unneccesary-dependency-array branch from 8ca766b to 7b754d0 Compare October 20, 2023 14:47
@paghar
Copy link
Contributor Author

paghar commented Oct 20, 2023

@rluders Please take a look at this, thanks....

@codecov
Copy link

codecov bot commented Oct 20, 2023

Codecov Report

Attention: 482 lines in your changes are missing coverage. Please review.

Comparison is base (7461173) 99.54% compared to head (5e2c18e) 93.56%.
Report is 128 commits behind head on main.

❗ Current head 5e2c18e differs from pull request most recent head af25447. Consider uploading reports for the commit af25447 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1079      +/-   ##
==========================================
- Coverage   99.54%   93.56%   -5.99%     
==========================================
  Files         163      205      +42     
  Lines        6621     8545    +1924     
  Branches      401      468      +67     
==========================================
+ Hits         6591     7995    +1404     
- Misses         30      550     +520     
Files Coverage Δ
src/components/Accordion/Accordion.tsx 100.00% <100.00%> (ø)
src/components/Accordion/AccordionContent.tsx 100.00% <100.00%> (ø)
src/components/Accordion/AccordionPanel.tsx 100.00% <100.00%> (ø)
src/components/Accordion/AccordionPanelContext.tsx 88.88% <100.00%> (+1.38%) ⬆️
src/components/Accordion/AccordionTitle.tsx 100.00% <100.00%> (ø)
src/components/Alert/Alert.tsx 100.00% <100.00%> (ø)
src/components/Avatar/Avatar.tsx 100.00% <100.00%> (ø)
src/components/Avatar/AvatarGroup.tsx 100.00% <100.00%> (ø)
src/components/Avatar/AvatarGroupCounter.tsx 100.00% <100.00%> (ø)
src/components/Badge/Badge.tsx 100.00% <100.00%> (ø)
... and 106 more

... and 40 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SutuSebastian
Copy link
Collaborator

@paghar I wouldn't really call it "unnecessary" tho, It is there more for performance because we want to mount the listeners once, not every single render. One single downside that I can think of is the callback closure and possibly stale/outdated callback variables if they have from one execution to another, but here with this implementation, it is not the case.

@rluders
Copy link
Collaborator

rluders commented Oct 26, 2023

Agreed with @SutuSebastian. I think that we don't need this. But, thanks anyway. :)

@rluders rluders closed this Oct 26, 2023
@paghar
Copy link
Contributor Author

paghar commented Oct 26, 2023

@rluders @SutuSebastian Thanks for the explanation, Is there any better solution to get rid of that warning?

@rluders
Copy link
Collaborator

rluders commented Oct 26, 2023

@paghar in this case, I think that we can just ignore it.

@rluders rluders reopened this Oct 26, 2023
@rluders
Copy link
Collaborator

rluders commented Oct 26, 2023

@paghar I'll reopen the PR so, maybe you can just add the line to ignore the warning and it should be all.

@paghar
Copy link
Contributor Author

paghar commented Oct 26, 2023

@paghar I'll reopen the PR so, maybe you can just add the line to ignore the warning and it should be all.

Thanks @rluders 🙏

@paghar paghar force-pushed the fix/remove-unneccesary-dependency-array branch from 7b754d0 to 5e2c18e Compare October 26, 2023 10:52
@paghar paghar force-pushed the fix/remove-unneccesary-dependency-array branch from 5e2c18e to af25447 Compare October 26, 2023 10:52
@paghar
Copy link
Contributor Author

paghar commented Oct 26, 2023

Done @rluders,
Thank you...

@rluders rluders merged commit 8ddebe1 into themesberg:main Oct 26, 2023
2 checks passed
@SutuSebastian
Copy link
Collaborator

@rluders @SutuSebastian Thanks for the explanation, Is there any better solution to get rid of that warning?

U're most welcome! Sure, there is a solution, that is putting the callback handleStorageChange in the dependency array, but this means that I have to memoise the callback everywhere I use it, its a bummer 🙄.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants