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(plugin): don't update props when plugin width or height props change #1406

Merged
merged 2 commits into from
Feb 11, 2025

Conversation

KaiVandivier
Copy link
Contributor

Accidentally introduced this in 3.13.1 -- useEffect dependencies retrigger props updates, which causes unnecessary refetches in plugins

Before:

before.mov

After:

after.mov

@KaiVandivier KaiVandivier requested a review from a team February 10, 2025 14:27
])
// Skip `width` and `height` props to avoid updates when changing size:
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [memoizedPropsToPass, inErrorState, alertsAdd, clientWidth])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this cause issues if width/height is provided and later removed (or vice versa)? The callbacks won't be sent to the child, even though it should switch to child-driven sizing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think instead there should be a dependency here on a boolean variable !height and another !width && clientWidth, or something similar

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it will -- it's not great, but I felt okay doing this because this is how it was before, too

That's also probably an unlikely case; someone would be best off using a className or a container for changeable styles like that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The boolean works 👌 added that!

])
// Skip `width` and `height` props to avoid updates when changing size:
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [memoizedPropsToPass, inErrorState, alertsAdd, clientWidth])

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of disabling the linter, what about having boolean state vars hasHeight and hasWidth and pass those instead to this effect?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that would work, good suggestion 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since height and width are props, it would take some 'prev/current' ref juggling to do

The boolean prop works though :)

@KaiVandivier
Copy link
Contributor Author

KaiVandivier commented Feb 10, 2025

Thanks for the tips; I was a bit hasty opening this 😅

Tested and still working

@KaiVandivier KaiVandivier requested a review from amcgee February 10, 2025 15:06
Copy link
Member

@amcgee amcgee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@KaiVandivier KaiVandivier merged commit d263c25 into master Feb 11, 2025
16 checks passed
@KaiVandivier KaiVandivier deleted the fix-unnecessary-prop-updates branch February 11, 2025 09:06
dhis2-bot added a commit that referenced this pull request Feb 11, 2025
## [3.13.2](v3.13.1...v3.13.2) (2025-02-11)

### Bug Fixes

* **plugin:** avoid sending prop updates when height and width values change ([#1406](#1406)) ([d263c25](d263c25))
@dhis2-bot
Copy link
Contributor

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

Successfully merging this pull request may close these issues.

4 participants