Improve speculation rules handling based on element visibility #446
Conversation
|
CI fails, please make sure tests are valid. |
|
TODO in order to complete merge this PR:
|
Changed "urlsToPrerender" property type on JSDoc Co-authored-by: Weston Ruter <[email protected]>
|
Thanks @giorgiopellegrino for adding the documentation changes, can you please add them as part of this PR? Context: giorgiopellegrino@f522eef |
Error return type object Co-authored-by: Weston Ruter <[email protected]>
const variable specRulesInViewport Co-authored-by: Weston Ruter <[email protected]>
Added functionality to the listen({ prerender: true }) method to allow prerendering only of content within the viewport, and introduced the eagerness property to the prerender method.
|
This is looking close to ready to merge! @giorgiopellegrino can we update it to be merge-friendly with the main branch and double check CI is passing? |
…into dynamic-speculationRules
Hi @addyosmani , |
Co-authored-by: Weston Ruter <[email protected]>
Co-authored-by: Weston Ruter <[email protected]>
Co-authored-by: Weston Ruter <[email protected]>
Co-authored-by: Weston Ruter <[email protected]>
Co-authored-by: Weston Ruter <[email protected]>
Co-authored-by: Weston Ruter <[email protected]>
|
A big thank you to everyone for the very thorough reviews on these changes and to @giorgiopellegrino for the additions. These LGTM to be merged and will be included in the next release. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -168,6 +178,9 @@ | |||
| if (index > -1) { | |||
| hrefsInViewport.splice(index); | |||
| } | |||
| if (specRulesInViewport.has(entry.href)) { | |||
| specRulesInViewport = removeSpeculationRule(specRulesInViewport, entry.href); | |||
Check failure
Code scanning / CodeQL
Assignment to constant Error
|
This shouldn't have landed as is; there's a syntax error as shown by CodeQL :/ |
|
Also, the size limit check failed, I increased in on main but should have been increased in this PR too. |
|
Oh, also the site docs should reflect the new addition. |
|
Makes me wonder why tests (not lint) didn't catch the const assignment. It's like tests don't cover this path... Regardless, the docs and the error should be fixed and in the future we better not merge when CI is red :) I also explicitly enabled the related ESLint rule on main. |
Pull Request Description:
This pull request addresses the issue raised in #442 by enhancing how speculation rules are managed based on the visibility of DOM elements.
Specifically, speculation rules are now added only when the target element is within the viewport and removed when it exits the viewport. This change enables developers to safely use
quicklink.listen(el, { prerender: true, eagerness: 'immediate' })on mobile devices without the risk of hitting the browser-imposed limit of 10 prerendered pages.This implementation improves resource management and ensures a more efficient and predictable prerendering behavior.
Thanks,
Giorgio Pellegrino.
cc @gilbertococchi