feat: fixed dimensions efficiency #1386

tomzemp · 2024-09-05T13:06:18Z

Key features

This PR cleans up some communication of height/width and related functions for resetting those dimensions to improve performance.

Description

When we discussed the api, we decided that:

specifying height/width would lead to the iframe for the plugin being fixed on that dimension. Leaving height/width out would cause the iframe to autoresize (callbacks in plugin communicate back height/width of contents and allow the iframe to resize)
for simplicity of implementation, I kept the height/width in state and communicated these props back to the plugin. However, keeping this in state leads to unnecessary renders for fixed-dimensions plugins, e.g. there is not a reason to keep height/width in a state variable and communcate changes in these values to the plugin.

This PR attempts to improve the implementation to separate fixed height/width (props) from resizedHeight/resizedWidth (in state, for plugins that do not provide a dimension and hence indicate that there should be autoresizing)

Checklist

Have written Documentation
- Documentation is updated to clarify that the height/width will not be communicated to the plugin by default.
Has tests coverage
- component needs tests in general 🙈

Known issues

In issue of simplicity, I am just updating app-runtime. The implementation in app-shell could also be cleaned up to improve some logic (https://github.com/dhis2/app-platform/blob/master/shell/src/PluginLoader.js#L173-L179).

At the moment, since I have updated the memoization to exclude checking for changes on height/width, there would be problems if you switched the plugin from having fixed height/width to having autoresized dimensions. I think that it is unlikely that one would update the component in that way (🤷‍♂️), and the extra implementation to handle this would defeat the purpose of stopping the communication of height/width for fixed-dimension plugins. This might be something we could revisit if there are objections?

In approaching this, I decided to be relatively minimal, though I do wonder if we should consider how we want to keep plugin logic aligned between app-runtime and app-platform aligned going forward.

tomzemp · 2024-09-05T13:07:28Z

services/plugin/src/Plugin.tsx

 }): JSX.Element => {
    const iframeRef = useRef<HTMLIFrameElement>(null)

-    // we do not know what is being sent in passed props, so for stable reference, memoize using JSON representation
-    const propsToPassNonMemoizedJSON = JSON.stringify(propsToPassNonMemoized)
-    const propsToPass: any = useMemo(


I removed this as I think it was unnecessary extra memoization, though it's not strictly related to the goal of the ticket.

edoardo

I've tested this in dashboard with both visualization and custom plugin items and it works.
In both cases we have fixed dimensions for the dashboard items (they are saved as part of the dashboard "layout") so for us the most important thing is that a resize of the window or the single dashboard item (edit mode) does not trigger unnecessary renders of the plugin content which is likely fetching data.
The analytics data doesn't change when only the dashboard item dimensions change.

KaiVandivier

Code looks functional 👍 thanks @edoardo for testing!

there would be problems if you switched the plugin from having fixed height/width to having autoresized dimensions

I think that's an okay tradeoff, and okay to do a slight "breaking change" since we consider the plugins experimental

Before approving, any reason height and width are still passed as props to the plugin? Removing those would simplify some of the propsToPass logic

Other thoughts unrelated to this particular PR:

Maybe unrelated functionally, but I recommend allowing the height and width props to take strings too -- I think a relatively common use case will be to use values like 100% or 50vh as values for those... I think number | string as a type should work and be backwards compatible, since { width: 100 } will use px by default 🤔
Some cleanup in the Plugin Loader would be good too -- I've noticed one prop update can cause 3 rerenders, I think due to the setState calls in receivePropsFromParent

tomzemp · 2024-09-06T12:40:21Z

Thanks @KaiVandivier 🙏

Before approving, any reason height and width are still passed as props to the plugin? Removing those would simplify some of the propsToPass logic

I left them in there because I thought it was more logical given that app-platform logic expects them (https://github.com/dhis2/app-platform/blob/master/shell/src/PluginLoader.js#L173-L179), but yes we don't need to pass them ever and the first part of the logical check (e.g. !height) will always be true and we just rely on whether setPluginHeight is defined to determine if the resize functions should be used.

Maybe unrelated functionally, but I recommend allowing the height and width props to take strings too -- I think a relatively common use case will be to use values like 100% or 50vh as values for those... I think number | string as a type should work and be backwards compatible, since { width: 100 } will use px by default 🤔

This is partly because with the resized height/width it will be pixels, so I guess it was easier conceptually to keep them the same, but that's not relevant now.

The other issue is that, at least for me, the iframe does not interpret a number without a unit (e.g. you need to specify "500px" or "50vw" (this might be browser dependent, but in general putting "height: 500" gets interpreted as "height: 500px" and that is not happening when I define an iframe):

I thought it would be easier to just have the definition be a number, but seems fair to expect that people would want to define the dimensions in other non-pixel units, so sure we could address that within this ticket. I've added in some logic to check if the dimension is a number without a unit, and if it is append 'px'; otherwise, return the original string (FYI: I confirmed that a number with percentage sign Number(90%) is NaN)

Some cleanup in the Plugin Loader would be good too -- I've noticed one prop update can cause 3 rerenders, I think due to the setState calls in receivePropsFromParent

Yes. There's definitely room for improvement 😄🫠

KaiVandivier · 2024-09-06T13:54:02Z

Ah interesting 😮 maybe numbers only work coming through React or something?

I also didn't get numbers to work when manipulating values using the dev tools, but providing a number in the code does seem to get converted to px:

I checked some autocomplete suggestions for some extra guidance, and they seem to tolerate numbers too (P.S., any thoughts about using the height and width properties on the iframe element? do those work well, or does the style have advantages?):

tomzemp · 2024-09-09T07:13:51Z

Thanks @KaiVandivier

I also didn't get numbers to work when manipulating values using the dev tools, but providing a number in the code does seem to get converted to px:

Huh. For me that does not work, and it's not converted to px directly in the code 🤔

I checked some autocomplete suggestions for some extra guidance, and they seem to tolerate numbers too (P.S., any thoughts about using the height and width properties on the iframe element? do those work well, or does the style have advantages?):

I don't really remember why I put them in style 🙃. I feel like there might have been a reason, but maybe it's now obsolete (there was some iterations on the resize logic, so maybe it was relevant for some earlier attempt). Anyway, I just checked with height/width directly on iframe and that seems to work fine (I just tested in chrome, firefox, opera; safari had authentication/cookie issues which made it not possible to test). Additionally, with the height/width directly on the iframe, I see 600 getting autoconverted to 600px, so then we get drop additional logic for ensuring that is the case and just let users pass string with or without a unit.

KaiVandivier

Nice! 🙂 One last thing then:

KaiVandivier · 2024-09-09T14:18:49Z

services/plugin/src/Plugin.tsx

+    height?: string
+    width?: string


Allow number as a prop type?

Suggested change

height?: string

width?: string

height?: number | string

width?: number | string

I don't know? I thought that it was more standard to define dimension as a string in the sense that it's supposed to include the unit: https://developer.mozilla.org/en-US/docs/Web/CSS/dimension

Are we seeing this as a compromise implementation? For example, update the prop type to take number, but not update the component documentation, such that we don't "advertise" this?

I guess that could be okay, as we probably don't want to promote numbers, since we're now relying on the iframe + browser implementation to convert 500 to 500px.

I don't actually see it as a compromise implementation; my intention is to match the behavior of the HTML iframe height attribute, which is a bit different than CSS requirements

It's a little hard to get formal documentation on what the accepted values actually are for iframe.height, but MDN at least indicates it can be a number, and the autocomplete in VSCode indicates it can be a number or a string

Out of curiosity, when passing just 500 to the height attribute and it didn't work to get converted to px in the browser styles, was it formatted as a string? I wonder if that makes a difference...

the mdn iframe reference says it’s a number (in pixels), but then the example shows a string-representation of a number, so I don't know at this point. It works for me to pass a number, a string-representation of a number, or a string with px or another unit specified. I don't really know what the limits are so I'll just update the type definition and leave the docs alone.

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/iframe#height

Why not update the docs? I think it makes sense to reflect the actual API

Does it still seem like a compromise to you? It seems standard to me since it mimics the iframe.height types, and based on our testing, seems well-supported -- I'm not sure why a number didn't work in one of your tests, but it seems a bit like a fluke

# [3.11.0](v3.10.6...v3.11.0) (2024-09-10) ### Features * fixed dimensions efficiency ([#1386](#1386)) ([2b07a14](2b07a14))

dhis2-bot · 2024-09-10T12:59:24Z

🎉 This PR is included in version 3.11.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

* fixed dimensions efficiency ([#1386](#1386)) ([2b07a14](2b07a14))

# [3.11.0-alpha.1](v3.10.4-alpha.1...v3.11.0-alpha.1) (2024-11-26) ### Bug Fixes * **cacheable-section:** synchronously flush recording state for UI consistency ([04bc604](04bc604)) * add endpoint to text plain matchers ([#1390](#1390)) ([de8fbec](de8fbec)) * expand FetchErrorDetails type ([#1389](#1389)) ([78ad0b3](78ad0b3)) * handle alert returned async by parentAlertsAdd [LIBS-695] ([#1388](#1388)) ([bba9c23](bba9c23)) * **cacheable-section:** stable references to avoid loops [LIBS-642] ([#1385](#1385)) ([31562e9](31562e9)) * update plugin sizing definition ([#1383](#1383)) ([38c09b9](38c09b9)) * **deps:** remove cli-app-scripts peer dep [LIBS-587] ([#1379](#1379)) ([9e22e88](9e22e88)) * **deps:** update cli-app-scripts for package/types race condition ([dee6795](dee6795)) ### Features * fixed dimensions efficiency ([#1386](#1386)) ([b56ad2d](b56ad2d)) * upgrade app-runtime React version to v18 ([#1387](#1387)) ([0e4a3d5](0e4a3d5))

feat: fixed dimensions efficiency

b574425

tomzemp commented Sep 5, 2024

View reviewed changes

tomzemp requested review from edoardo, kabaros, d-rita and KaiVandivier September 5, 2024 13:26

edoardo reviewed Sep 5, 2024

View reviewed changes

KaiVandivier reviewed Sep 5, 2024

View reviewed changes

feat: pr feedback

5868050

fix: move height/width out of styles

737b238

KaiVandivier approved these changes Sep 9, 2024

View reviewed changes

tomzemp added 3 commits September 10, 2024 12:48

fix: update prop types

f8b5d98

fix: update upload-artifact

711a520

fix: update download-artifact

722da5f

tomzemp merged commit 2b07a14 into master Sep 10, 2024
14 checks passed

tomzemp deleted the dimensions-performance branch September 10, 2024 12:47

dhis2-bot added a commit that referenced this pull request Sep 10, 2024

chore(release): cut 3.11.0 [skip ci]

1713f0f

# [3.11.0](v3.10.6...v3.11.0) (2024-09-10) ### Features * fixed dimensions efficiency ([#1386](#1386)) ([2b07a14](2b07a14))

dhis2-bot added the released label Sep 10, 2024

KaiVandivier pushed a commit that referenced this pull request Nov 25, 2024

feat: fixed dimensions efficiency (#1386)

b56ad2d

KaiVandivier pushed a commit that referenced this pull request Nov 25, 2024

chore(release): cut 3.11.0 [skip ci]

745706c

* fixed dimensions efficiency ([#1386](#1386)) ([2b07a14](2b07a14))

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: fixed dimensions efficiency #1386

feat: fixed dimensions efficiency #1386

tomzemp commented Sep 5, 2024 •

edited by jira bot

Loading

tomzemp Sep 5, 2024

edoardo left a comment •

edited

Loading

KaiVandivier left a comment •

edited

Loading

tomzemp commented Sep 6, 2024 •

edited

Loading

KaiVandivier commented Sep 6, 2024

tomzemp commented Sep 9, 2024 •

edited

Loading

KaiVandivier left a comment

KaiVandivier Sep 9, 2024

tomzemp Sep 9, 2024

KaiVandivier Sep 10, 2024 •

edited

Loading

tomzemp Sep 10, 2024

KaiVandivier Sep 10, 2024

dhis2-bot commented Sep 10, 2024

feat: fixed dimensions efficiency #1386

feat: fixed dimensions efficiency #1386

Conversation

tomzemp commented Sep 5, 2024 • edited by jira bot Loading

Key features

Description

Checklist

Known issues

In approaching this, I decided to be relatively minimal, though I do wonder if we should consider how we want to keep plugin logic aligned between app-runtime and app-platform aligned going forward.

tomzemp Sep 5, 2024

Choose a reason for hiding this comment

edoardo left a comment • edited Loading

Choose a reason for hiding this comment

KaiVandivier left a comment • edited Loading

Choose a reason for hiding this comment

tomzemp commented Sep 6, 2024 • edited Loading

KaiVandivier commented Sep 6, 2024

tomzemp commented Sep 9, 2024 • edited Loading

KaiVandivier left a comment

Choose a reason for hiding this comment

KaiVandivier Sep 9, 2024

Choose a reason for hiding this comment

tomzemp Sep 9, 2024

Choose a reason for hiding this comment

KaiVandivier Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

tomzemp Sep 10, 2024

Choose a reason for hiding this comment

KaiVandivier Sep 10, 2024

Choose a reason for hiding this comment

dhis2-bot commented Sep 10, 2024

tomzemp commented Sep 5, 2024 •

edited by jira bot

Loading

edoardo left a comment •

edited

Loading

KaiVandivier left a comment •

edited

Loading

tomzemp commented Sep 6, 2024 •

edited

Loading

tomzemp commented Sep 9, 2024 •

edited

Loading

KaiVandivier Sep 10, 2024 •

edited

Loading