-
Notifications
You must be signed in to change notification settings - Fork 65
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
Prevent unnecessary nesting of breakpoint nodes (fixes #132) #158
Prevent unnecessary nesting of breakpoint nodes (fixes #132) #158
Conversation
feat: working deep nesting prevention with initial passing test
Ready for review! We have run this on our large project which we recently refactored to use fresnel and the HTML document size reduced from 30mb to 60kb. The 30mb was caused a large base64 image (generated with next-optimized-images) being duplicated hundreds of times when they'd never be rendered in the first place. All other pages were reduced in size dramatically and we've seen no bugs or layout defects as a result. Our issue was dramatic as we needed to do a full project refactor, I think if you were using fresnel from the start you'd likely be cautious not to over-use. We refactored our core grid layout component to use fresnel which was 3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is so exciting! 👏
@damassi Can you give this a try on the Artsy site?
Co-authored-by: Eloy Durán <eloy.de.enige@gmail.com>
Cheers for the review @alloy, FYI I had forgotten to push some extra tests up to this branch, I've just done so which was likely after your review. |
Yup, i'll update in an hour and get back to y'all. Nice update @kirkness 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed that things look good over here 👍 Thanks for the great PR.
🚀 PR was released in |
Thanks for the contribution and the diligent testing, @kirkness 🙏 |
This PR introduces a mechanism that prevents children of
Media
from ever rendering if there is no possible way of them rendering - hopefully, a fix for #132. We recently refactored an app to use this library and noticed that the amount of duplicated nodes being rendered was causing performance downside.Example:
This would result in the server returning 4 of the large
<img />
strings, where we could logically just return 1, given the parent at the root of the tree is ansm
. The output should ideally be:The update works using context and by wrapping each
Media
in aProvider
, the childMedia
's can then check to see whether there are any conflicting parents that would prevent it from rendering in the first place.We'll install this fork in our project and I'll shift this out of draft once we are confident it's working.