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

FR: allow doc level opt-out of amp-auto-lightbox #26452

Closed
photocoder opened this issue Jan 23, 2020 · 10 comments · Fixed by #37854
Closed

FR: allow doc level opt-out of amp-auto-lightbox #26452

photocoder opened this issue Jan 23, 2020 · 10 comments · Fixed by #37854

Comments

@photocoder
Copy link

What's the issue?

My website use AMP. And home page doesn't have any "lightbox" pluggins added.
even more, I put "data-amp-auto-lightbox-disable" tag everywhere I can, but, I still see in information, that it's loaded.
"amp-auto-lightbox-0.1.js"
You can see full stat with Waterfall Chart of request-by-request visualization of the page load here: https://gtmetrix.com/reports/dimasfrolov.com/bR3vy8mq

How do we reproduce the issue?

https://dimasfrolov.com/

What browsers are affected?

Chrome, Opera, Firefox

Which AMP version is affected?

Powered by AMP ⚡ HTML – Version 2001071857360

@caroqliu
Copy link
Contributor

caroqliu commented Jan 27, 2020

The data-amp-auto-lightbox-disable tag disables behavior of the lightbox but not inclusion of the script in the first place. This seems like a feature request to be able to do the latter. /cc @ampproject/wg-runtime @alanorozco

@caroqliu caroqliu changed the title data-amp-auto-lightbox-disable doesn't work. Please help! FR: allow doc level opt-out of amp-auto-lightbox Jan 27, 2020
@alanorozco
Copy link
Member

@photocoder amp-auto-lightbox-0.1.js is tiny and has no effect when disabled. Is there a particular need you're trying to fulfill by not loading it?

@jitendravyas
Copy link

jitendravyas commented Feb 7, 2020

I have the same question. Why should it load if I don't intend to use. It's 3.4kb on wire.

@photocoder
Copy link
Author

photocoder commented Feb 7, 2020

@photocoder amp-auto-lightbox-0.1.js is tiny and has no effect when disabled. Is there a particular need you're trying to fulfill by not loading it?

It just one more request what I didn't ask.
But anyway, if "disable" option exist, why it doesn't work?

@alanorozco
Copy link
Member

alanorozco commented Feb 7, 2020

@photocoder @jitendravyas

I'd like to understand the practical concerns coming from loading a script that is a) 3.4kb and b) to an already preconnected domain. Some clarification might be needed:

  1. Out of the 3.4kb size, ~40% is common extension overhead. This is being handled separately for all extensions, but in any case the difference is negligible for all practical purposes.

  2. Disabling per the data- attribute doesn't promise that the extension won't load. It means the treatment is not applied. Should this be clarified on docs?

  3. There's logic inside auto-lightbox to determine other criteria for treatment as well.

One important distinction between the core runtime binary (v0.js) and extension binaries (amp-auto-lightbox-*.js) is that the former blocks page display, while extensions don't delay the page from displaying.

Including auto-lightbox in the core runtime would make it ~2% larger. The page display delay is not a great trade-off since it would block every AMP page, while loading a separate binary won't delay visibility.

Otherwise, to introduce some block for the loading of the auto-lightbox binary itself we'd need to increase core size to some degree.

This might need some design work in order to work properly from the core runtime's pov. I'm also not fully convinced of the need to increase core size in order to prevent this request in general.

@ithinkihaveacat
Copy link
Contributor

For me it's not necessarily about the real-world perf implications, it's that loading the script just seems a bit unnecessary, wasteful and surprising. I certainly didn't expect it to be loaded even if the page contains no images (I actually thought this was a bug); in this case I don't think an opt out should be necessary, it should just not load at all.

@sebastianbenz
Copy link
Contributor

+1 to @ithinkihaveacat

I agree that it doesn't make sense from a perf point of view, but it is very surprising for developers and this question will come again and again.

@westonruter
Copy link
Member

We get support forum topics about this for the WordPress plugin as well.

@jridgewell
Copy link
Contributor

ping @nainar

@jonoalderson
Copy link
Contributor

jonoalderson commented May 6, 2021

Also keen to see this not load, when not needed/used. This waterfall is so very nearly perfect.

image

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