Skip to content

Adjusting memoization on the DashWrapper to be utilized #3248

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

Merged
merged 34 commits into from
Apr 1, 2025

Conversation

BSd3v
Copy link
Contributor

@BSd3v BSd3v commented Mar 26, 2025

This is a PR to test the adjustment of memoization on the DashWrapper

you can see info here: #3243

@BSd3v
Copy link
Contributor Author

BSd3v commented Mar 26, 2025

The initial test failed due to not being able to redraw components if the parent is adjusted.

The current test for this hash is startsWith I have updated it to render the first descendent of the updatedPath, however, this will not work if the desired outcome is to render and adjustment to every upstream component.

If the above is the desired outcome, I recommend having devs add a new prop to components that want to redraw on upstream adjustments and using that to switch to the older function which uses startsWith without any limit. Applying this to all descendants does not seem efficient.


Heck, even adding it as a prop that Dash adds would work.

@ndrezn ndrezn linked an issue Mar 26, 2025 that may be closed by this pull request
@BSd3v
Copy link
Contributor Author

BSd3v commented Mar 27, 2025

There was an issue with the useSelector in Dash 3, whenever a parent element was adjusted, it would cause all children to rerender. The higher up the tree, the more components had to rerender.

In this PR, we adjusted the reducer to trigger a layout adjustment only if the direct descendants are effected, ie, children prop was adjusted. The useSelector is now much narrow, only listening to adjustments on the specific component.

A couple of other things that were minor

  • fixed the issue where c was undefined and thus c.props never resolved
  • fixed the issue where setProps was triggered from a component and the component was no longer in the dash layout.

@BSd3v
Copy link
Contributor Author

BSd3v commented Mar 29, 2025

Another thing on the last comment, I adjusted the way the memoizing works, it only holds onto the last rendered time. This helps with rerenders.

@BSd3v
Copy link
Contributor Author

BSd3v commented Mar 31, 2025

I added renderType as an optional prop for developers. It allows the developer to be able to know what triggered the render.

i.e. when using an api component and setProps was triggered, the base component often doesnt need to rerender. To avoid this, the dev can check for props.renderType !== 'internal' to not rerender. 😄

BSd3v added 2 commits March 31, 2025 10:35
…ibe, they need to place on the component: `namespace.component.dashRenderType = true`
Copy link
Contributor

@T4rk1n T4rk1n left a comment

Choose a reason for hiding this comment

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

Nice job, just a few minor comments.

selectDashProps(componentPath),
selectDashPropsEqualityFn
);

/* eslint-disable @typescript-eslint/ban-ts-comment, @typescript-eslint/no-unused-vars */
// @ts-ignore
const newlyRendered = useMemo(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we put as newlyRendered: any instead of the ts-ignore?

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'll check with the @ts-ignore the unused vars needs to be ignored.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still errors out in the build.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I don't think useMemo is right in this case, should be useEffect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

useEffect is not sync, while useMemo is sync. For the current to be used within the same sync it has to be useMemo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Went with useMemo without variable declaration

@@ -41,24 +46,61 @@ type DashWrapperProps = {
*/
componentPath: DashLayoutPath;
_dashprivate_error?: any;
_passedComponent?: any;
_newRender?: any;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a comment describing those new props.

Copy link
Contributor

@T4rk1n T4rk1n left a comment

Choose a reason for hiding this comment

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

💃

@T4rk1n T4rk1n merged commit 8f585b5 into plotly:dev Apr 1, 2025
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community community contribution fix fixes something broken P1 needed for current cycle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prop tests in Debug=True slow when manipulating lots of children
3 participants