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

Universal IntersectionObserver polyfill #27595

Merged
merged 17 commits into from
Apr 22, 2020
Merged

Conversation

dvoytenko
Copy link
Contributor

@dvoytenko dvoytenko commented Apr 6, 2020

Partial for: #27807

This is a two-stage polyfill. It uses the asynchronous nature of IntersectionObserver API:

  1. v0.js creates a minimal stub. It only tracks requested intersection observers, but doesn't do any work.
  2. v0.js requests the polyfill via a dedicated extension
  3. Once the extension is loaded, it upgrades the stub to a full-featured polyfill.

TODO:

@kristoferbaxter
Copy link
Contributor

For the module build, can we ensure this polyfill is only loaded if the user-agent doesn't support IntersectionObserver and 'isIntersecting' in window.IntersectionObserverEntry.prototype?

@jridgewell
Copy link
Contributor

We're loading the polyfill if needed in both modes. Note that module builds need it, too: https://prateekbh.lib.id/caniesmodule/?feature=intersectionobserver

@kristoferbaxter
Copy link
Contributor

The vast majority of module browsers have support for IntersectionObserver, late loading it for a few small holdouts doesn't seem like a stretch.

@jridgewell
Copy link
Contributor

That's what we're doing, for both modes.

@kristoferbaxter
Copy link
Contributor

Missed that when reviewing, thanks for the confirmation.

@dvoytenko
Copy link
Contributor Author

For the module build, can we ensure this polyfill is only loaded if the user-agent doesn't support IntersectionObserver and 'isIntersecting' in window.IntersectionObserverEntry.prototype?

IMHO, isIntersecting polyfill is so tiny, it'd probably be best to keep it in v0.js, nomodule if we can be certain module supports it everywhere.

@dvoytenko
Copy link
Contributor Author

Heads up. This is slightly blocked by w3c/IntersectionObserver#419. But review is in progress and I expect it to be completed soon.

@dvoytenko
Copy link
Contributor Author

@erwinmombay @jridgewell PTAL, I added the actual polyfill, including rewriting the installer for controlled installation.

src/runtime.js Outdated Show resolved Hide resolved
src/polyfills/intersection-observer.js Outdated Show resolved Hide resolved
src/polyfills/intersection-observer.js Outdated Show resolved Hide resolved
src/polyfills/intersection-observer.js Show resolved Hide resolved
src/polyfills/intersection-observer.js Outdated Show resolved Hide resolved
src/polyfills/intersection-observer.js Outdated Show resolved Hide resolved
src/polyfills/intersection-observer.js Outdated Show resolved Hide resolved
src/polyfills/intersection-observer.js Outdated Show resolved Hide resolved
@dvoytenko
Copy link
Contributor Author

@jridgewell ptal again.

@dvoytenko
Copy link
Contributor Author

Upgraded to polyfill 0.8.0 that supports same-origin iframes (FIE).

src/polyfills.js Show resolved Hide resolved
build-system/global-configs/experiments-config.json Outdated Show resolved Hide resolved
src/runtime.js Outdated Show resolved Hide resolved
src/polyfills/intersection-observer.js Outdated Show resolved Hide resolved
@dvoytenko dvoytenko marked this pull request as ready for review April 17, 2020 01:15
@dvoytenko
Copy link
Contributor Author

All. This is now ready for review.

@dvoytenko
Copy link
Contributor Author

@zhouyx PTAL again.

src/polyfills/intersection-observer.js Outdated Show resolved Hide resolved
@dvoytenko dvoytenko requested a review from zhouyx April 21, 2020 00:46
@rcebulko
Copy link
Contributor

@danielrozenberg Do I just approve this? Shouldn't Dima have approval ability? I can't even tell why it's complaining, it looks like the check ran, but I can't tell what change it doesn't like

@danielrozenberg
Copy link
Member

@rcebulko it's possible that you were added for a bundle-size bot failure on a previous commit, and the force-push caused the check to run again, successfully this time. The PR author can't approve their own bundle-size change though, so someone else from the two listed teams needs to approve this PR again

@dvoytenko dvoytenko removed the request for review from rcebulko April 21, 2020 18:20
@dvoytenko
Copy link
Contributor Author

@rcebulko I'll take you off the review here. I think the reason you were added has been already resolved.

Copy link

@dreamofabear dreamofabear left a comment

Choose a reason for hiding this comment

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

Just some readability suggestions. Curious to try this with "intersect-resources" experiment.

*/

/**
* @fileoverview

Choose a reason for hiding this comment

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

Does this need to be a separate file from src/polyfills/intersection-observer.js? Following the code flow between the three files is a bit confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a bit confusing, I agree. I did this because otherwise src/polyfills are removed from compiler in many cases. I'd have to instead add a lot of overrides for js and mjs. And I figured that would be more dangerous rather than having a clear line between stubs and polyfill installers. But lmk if you think it'd still be better to live with exceptions.

/**
* @param {!Window} win
*/
export function install(win) {

Choose a reason for hiding this comment

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

I think the word "install" is used to mean "stub", "fetch" and "upgrade" in different places which is tough for readability. Can we call this stub?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed all others to upgrade per your suggestions above. But I'd rather keep calling this one install() b/c I think this hides the fact of stubbing from most of places as an implementation choice. But I also redid the "fetch" part as well to reduce number of entities. PTAL.

Copy link
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

build-system/ changes LGTM.

build-system/tasks/update-packages.js Outdated Show resolved Hide resolved
@erwinmombay erwinmombay self-requested a review April 22, 2020 01:06
@dvoytenko dvoytenko merged commit 419131a into ampproject:master Apr 22, 2020
danielrozenberg added a commit to danielrozenberg/amphtml that referenced this pull request Apr 22, 2020
@danielrozenberg
Copy link
Member

This experiment breaks releases, I created #27937 to revert the experiment config (none of the other files are being reverted)

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