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

Prevent unnecessary nesting of breakpoint nodes (fixes #132) #158

Merged
merged 6 commits into from Oct 26, 2020

Conversation

kirkness
Copy link
Contributor

@kirkness kirkness commented Oct 20, 2020

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:

<Media at="sm">
  <Media at="sm">
    <Media at="sm">
      <img src="base64:mymassssiveimage" />
    </Media>
    <Media at="md">
      <img src="base64:mymassssiveimage" />
    </Media>
  </Media>
  <Media at="md">
    <Media at="sm">
      <img src="base64:mymassssiveimage" />
    </Media>
    <Media at="md">
      <img src="base64:mymassssiveimage" />
    </Media>
  </Media>
</Media>

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 an sm. The output should ideally be:

<div class="fresnel-container fresnel-at-sm">
  <div class="fresnel-container fresnel-at-sm">
    <div class="fresnel-container fresnel-at-sm"><img src="base64:mymassssiveimage" /></div>
  </div>
</div>

The update works using context and by wrapping each Media in a Provider, the child Media'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.

feat: working deep nesting prevention with initial passing test
@kirkness kirkness marked this pull request as ready for review October 26, 2020 11:12
@kirkness
Copy link
Contributor Author

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 Media nodes deep, hence the vast document size.

Copy link
Collaborator

@alloy alloy left a 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?

src/__test__/Breakpoints.test.tsx Outdated Show resolved Hide resolved
src/__test__/Media.test.tsx Show resolved Hide resolved
src/__test__/Media.test.tsx Outdated Show resolved Hide resolved
src/__test__/Media.test.tsx Outdated Show resolved Hide resolved
Co-authored-by: Eloy Durán <eloy.de.enige@gmail.com>
@kirkness
Copy link
Contributor Author

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.

@damassi
Copy link
Member

damassi commented Oct 26, 2020

Yup, i'll update in an hour and get back to y'all. Nice update @kirkness 👍

Copy link
Member

@damassi damassi left a 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.

@damassi damassi merged commit cd95ee6 into artsy:master Oct 26, 2020
@artsyit
Copy link
Contributor

artsyit commented Oct 26, 2020

🚀 PR was released in v1.3.0 🚀

@alloy
Copy link
Collaborator

alloy commented Oct 27, 2020

Thanks for the contribution and the diligent testing, @kirkness 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants