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

Improving design section JS binding performance #15431

Merged
merged 9 commits into from
Jan 24, 2025

Conversation

mike12345567
Copy link
Collaborator

Description

This PR introduces vm-browserify as a direct dependency, rather than receiving this as part of the string-templates build process and a browser polyfill for the vm Node library.

I have forked the library to add an optimisation to re-use the iframe when processing JS - previously each JS binding on the page would add an iframe, run the JS and then destroy said iframe - from our testing this has some particularly large performance costs.

The difference can be seen here, when rendering a page with a repeater block, containing a headline with a return 1 JS binding - this is for 1000 components.

Before the library update:
image

After the library update:
image

Here we can see the total time to prepare the data for rendering has gone from 32.8 seconds to 1.4 seconds - a massive improvement in performance.

We have also introduced support for the iframe sandbox capabilities that are now generally supported by browsers (not an option when the library was originally built) - information can be found here: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/iframe

This restricts the JS binding from performing certain actions like opening a popover - we have limited it to allow-scripts and allow-same-origin sandboxing behaviours.

The re-use of the iframe did raise a question of global state management, however after much testing it was found that there is little difference, since the parent body is always accessible in the JS via window.parent - this means there has always been the potential of global state.

@mike12345567 mike12345567 self-assigned this Jan 23, 2025
Copy link

qa-wolf bot commented Jan 23, 2025

QA Wolf here! As you write new code it's important that your test coverage is keeping up.
Click here to request test coverage for this PR!

1 similar comment
Copy link

qa-wolf bot commented Jan 23, 2025

QA Wolf here! As you write new code it's important that your test coverage is keeping up.
Click here to request test coverage for this PR!

@github-actions github-actions bot added the firestorm Data/Infra/Revenue Team label Jan 23, 2025
@github-actions github-actions bot added size/s and removed size/xs labels Jan 23, 2025
Copy link
Member

@aptkingston aptkingston left a comment

Choose a reason for hiding this comment

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

🚀 I love that reusing the iframe had such a big impact. Definitely opens the doors for introducing bindings into grids for things like custom columns, and also hopefully lets me simplify some client library logic, which was extremely heavily optimised (at the cost of complexity) to minimize binding evaluation at all costs.

@mike12345567 mike12345567 merged commit c90296d into master Jan 24, 2025
20 checks passed
@mike12345567 mike12345567 deleted the feature/frontend-js-performance branch January 24, 2025 14:45
@github-actions github-actions bot locked and limited conversation to collaborators Jan 24, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
firestorm Data/Infra/Revenue Team size/s
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants