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

Add support for same-site prerendering with Speculation Rules API #258

Merged
merged 21 commits into from Aug 5, 2022
Merged

Add support for same-site prerendering with Speculation Rules API #258

merged 21 commits into from Aug 5, 2022

Conversation

hadyan
Copy link
Contributor

@hadyan hadyan commented Apr 26, 2022

  • As a new "prerender" option for quicklink.listen to prerender link(s) in the viewport with fallback to prefetching on unsupported browsers.
  • Create a new API quicklink.prerender to define static url(s) to prerender.

Closes #253

@google-cla
Copy link

google-cla bot commented Apr 26, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/index.mjs Outdated Show resolved Hide resolved
src/prerender.mjs Outdated Show resolved Hide resolved
Co-authored-by: Domenic Denicola <d@domenic.me>
src/index.mjs Outdated Show resolved Hide resolved
src/index.mjs Outdated Show resolved Hide resolved
removed extra argument in the promise constructor
fixed the inconsistent application of spacing throughout repo
src/index.mjs Outdated Show resolved Hide resolved
if (/2g/.test(conn.effectiveType)) {
return Promise.reject(new Error('Cannot prefetch, network conditions are poor'));
}
let chkConn = checkConnection(conn = navigator.connection);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear why this is assigning to the conn parameter. Did you mean to set a default on line 201? Or just remove the conn parameter altogether? Or do conn || navigator.connection?

I guess this was already buggy...

* @return {Object} a Promise
*/
export function prerender(urls, conn) {
let chkConn = checkConnection(conn = navigator.connection);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same problem with assigning to a parameter here.

src/index.mjs Outdated Show resolved Hide resolved
src/index.mjs Outdated Show resolved Hide resolved
src/prerender.mjs Outdated Show resolved Hide resolved
src/prerender.mjs Show resolved Hide resolved
Copy link
Collaborator

@addyosmani addyosmani left a comment

Choose a reason for hiding this comment

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

LGTM

@addyosmani addyosmani merged commit 3d26f40 into GoogleChromeLabs:master Aug 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add same-site prerendering with speculation rule
3 participants