Skip to content

fix(plugin): don't update props when plugin width or height props change#1406

Merged
KaiVandivier merged 2 commits intomasterfrom
fix-unnecessary-prop-updates
Feb 11, 2025
Merged

fix(plugin): don't update props when plugin width or height props change#1406
KaiVandivier merged 2 commits intomasterfrom
fix-unnecessary-prop-updates

Conversation

@KaiVandivier
Copy link
Copy Markdown
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
Comment thread services/plugin/src/Plugin.tsx Outdated
])
// 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
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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!

Comment thread services/plugin/src/Plugin.tsx Outdated
])
// 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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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 :)

@sonarqubecloud
Copy link
Copy Markdown

@KaiVandivier
Copy link
Copy Markdown
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
Copy Markdown
Contributor

@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
@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
Copy Markdown
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