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 renderDelay not being applied to dynamicBlocks #3595

Closed
wants to merge 1 commit into from

Conversation

cmdcolin
Copy link
Collaborator

@cmdcolin cmdcolin commented Mar 18, 2023

Currently, dynamicBlocks rapidly re-render themselves on scroll which can cause some issues if the rendering is an expensive routine

the blocks currently render themselves automatically 'afterAttach' inside the serverSideRenderedBlock code. this PR changes it so that the BaseLinearDisplayModel will render them after an autorun delay (set to renderDelay), so the rendering is then controlled not by the block's automatic afterAttach behavior, but by the display model itself.

Blocks are not re-rendered if they are already "filled"

Fixes #887

part of a possible effort to do the 'variant matrix display'

@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Mar 18, 2023
@cmdcolin cmdcolin force-pushed the dynamic_blocks_renderdelay branch from 38e9b88 to 16fc4f5 Compare March 18, 2023 20:37
@cmdcolin cmdcolin added bug Something isn't working and removed needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) labels Mar 18, 2023
@codecov
Copy link

codecov bot commented Mar 18, 2023

Codecov Report

Merging #3595 (d4c1065) into main (4c6a784) will decrease coverage by 0.16%.
The diff coverage is 92.53%.

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

@@            Coverage Diff             @@
##             main    #3595      +/-   ##
==========================================
- Coverage   63.06%   62.91%   -0.16%     
==========================================
  Files         871      866       -5     
  Lines       30189    30188       -1     
  Branches     7269     7269              
==========================================
- Hits        19039    18992      -47     
- Misses      10965    11007      +42     
- Partials      185      189       +4     
Impacted Files Coverage Δ
...ableElementTypes/renderers/RpcRenderedSvgGroup.tsx 0.00% <0.00%> (ø)
...ementTypes/renderers/ServerSideRenderedContent.tsx 0.00% <0.00%> (ø)
...ments/src/LinearAlignmentsDisplay/models/model.tsx 80.17% <0.00%> (ø)
...r-genome-view/src/BaseLinearDisplay/models/util.ts 82.60% <93.33%> (ø)
...aseLinearDisplay/models/serverSideRenderedBlock.ts 96.07% <95.78%> (+0.84%) ⬆️
packages/core/util/index.ts 84.08% <100.00%> (+0.99%) ⬆️
...nts/src/SNPCoverageRenderer/SNPCoverageRenderer.ts 74.40% <100.00%> (ø)
...c/BaseLinearDisplay/components/TooLargeMessage.tsx 100.00% <100.00%> (ø)
...aseLinearDisplay/models/BaseLinearDisplayModel.tsx 88.50% <100.00%> (ø)
plugins/variants/src/VariantTrack/configSchema.ts 100.00% <100.00%> (+33.33%) ⬆️

... and 49 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@cmdcolin
Copy link
Collaborator Author

the approach to only redraw if "!filled" did not respond to some state tree changes (but surprisingly passed all tests). for example, changing 'Show translation' on sequence track did not update the rendering

so, reverted that change and more basically added a setTimeout(...,renderDelay) before registering the render reaction inside of serverSideRenderedBlock

@@ -659,7 +658,7 @@ export function makeAbortableReaction<T, U, V>(
return undefined
}
},
async (data, mobxReactionHandle) => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

unless the mobxReactionHandle is very useful to pass to the renderReaction, seems like we can remove it. avoids the typescript error

@cmdcolin cmdcolin force-pushed the dynamic_blocks_renderdelay branch 7 times, most recently from 9878e72 to 18c2185 Compare March 20, 2023 17:21
@cmdcolin cmdcolin mentioned this pull request Mar 20, 2023
@cmdcolin cmdcolin force-pushed the dynamic_blocks_renderdelay branch from 18c2185 to f6eda27 Compare March 20, 2023 17:37
@cmdcolin
Copy link
Collaborator Author

this PR now includes a 'new approach to the block hydration'

I saw a persistent error that said this "Node.removeChild: The node to be removed is not a child of this node", it was rare but if i scrolled really fast for awhile it could be reproduced. this was quite odd, and I could not find much info online for it.

Also, if I removed the requestIdleCallback (rIC) from the hydration, it caused several other errors, which indicated to me that maybe there was something maybe wrong with our approach...at the very least, it seemed to be that it required a process tick and was not just there to 'improve our frame rate' as a comment in the code suggested

I made a stackoverflow question to see if there was any hivemind knowledge about this https://stackoverflow.com/questions/75786709/how-can-you-properly-use-hydrate-and-or-hydrateroot-inside-of-an-existing-reac, but then possibly I made an alternative solution that may work instead: using dangerouslySetInnerHTML instead of changing domNode.innerHTML, and then NOT using unmountComponentAtNode at all. the thinking is the node is fully owned by the react tree and does not need it. if I add unmountComponentAtNode back as e.g. the destructor of the useEffect, then I get errors like ('Warning: unmountComponentAtNode(): The node you're attempting to unmount was rendered by another copy of React.'). as far as i can tell, i did not see any memory issues from removing unmountComponentAtNode (e.g. did not affect #3596)

@cmdcolin
Copy link
Collaborator Author

made this PR only related to block renderDelay and the hydration changes now

@cmdcolin cmdcolin marked this pull request as draft March 24, 2023 04:47
@cmdcolin cmdcolin force-pushed the dynamic_blocks_renderdelay branch 4 times, most recently from a3b06df to 822162b Compare April 10, 2023 01:47
@cmdcolin cmdcolin force-pushed the dynamic_blocks_renderdelay branch from 822162b to fa3e69f Compare April 10, 2023 01:47
@cmdcolin cmdcolin marked this pull request as ready for review April 10, 2023 01:47
@cmdcolin
Copy link
Collaborator Author

a potential fix for the hydration error was added. i only unmountComponentAtNode after setTimeout(0) which in my testing did not create any more warnings about things like "Warning: unmountComponentAtNode(): The node you're attempting to unmount was rendered by another copy of React."...answered my own stackoverflow here https://stackoverflow.com/questions/75786709/how-can-you-properly-use-hydrate-and-or-hydrateroot-inside-of-an-existing-reac

@cmdcolin
Copy link
Collaborator Author

ultimately, the hydration changes and other refactorings produce intermittent hydration errors and react errors. it's hard to understand. I don't believe our current setup is the most idiomatic, but I have not seen these errors with our current system. therefore, leaving current system untouched. can close this for now and revisit in future perhaps

@cmdcolin cmdcolin closed this Apr 11, 2023
@cmdcolin cmdcolin deleted the dynamic_blocks_renderdelay branch June 8, 2023 02:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dynamic blocks renderDelay
1 participant