backporting the root implementation with memoize feature by lynchbomb · Pull Request #80 · linkedin/spaniel

lynchbomb · 2018-02-20T23:10:48Z

Spike for back-porting the v3 root implementation within Intersection-Observer.
Handling occluded elements and child scroll containers.

asakusuma

I’ll take another pass later. A few questions:

Does this change handle nested roots?
I assume this handles the scenario where some other code captures the window scroll event?

asakusuma · 2018-02-23T00:21:18Z

USAGE.md

 * `time` - The time threshold
 * `ratio` - The ratio threshold
-* `rootMargin` - The [rootMargin](https://wicg.github.io/IntersectionObserver/#dom-intersectionobserverinit-rootmargin) in object form.
+* `rootMargin` - The rootMargin in object form.


Why get rid of the link

It's 404-ing. FWIW rootMargin is as well. Just need to find the new relative links for these.

maybe https://developer.mozilla.org/en-US/docs/Web/API/Intersection_Observer_API#Intersection_observer_options ?

asakusuma · 2018-02-23T00:22:54Z

src/metal/scheduler.ts

-  static generate(): Frame {
+  static generate(root?: Element): Frame {
+    if (root && document.documentElement.contains(root)) {
+      let rootClientRect = getBoundingClientRect(root);


Can this be a const?

asakusuma · 2018-02-23T00:24:03Z

src/metal/scheduler.ts

  ) {}
-  static generate(): Frame {
+  static generate(root?: Element): Frame {
+    if (root && document.documentElement.contains(root)) {


In the past, we have used the window proxy for any direct touching of the document or the window, so that things don’t break in node land. Is this a safe direct document reference?

Nice catch. Besides the private conditionals within the window proxy for hasDomSetup and hasDOM we aren't currently (to my knowledge) checking for DOM before leveraging the document. We should definitely think about exporting a document proxy or even just a document interface to check for DOM etc.

asakusuma · 2018-02-23T00:29:27Z

src/metal/window-proxy.ts

-    windowSetScrollMeta();
-  }, throttleDelay);
+// Only invalidate window isDirty on scroll and resize
+function eventThrottle(eventType?: string, root?: Element) {


Does consolidating into one throttle function buy us anything if we are still switching on each case?

Nothing major. One less function declaration in a succinct easier to read package.

I would strongly recommend using 2 discrete methods.

Rationale (for 2 functions)

more concise (less bytes)

less prone to error (if an invalid event type is used, we get a useful error)

remove unneeded wrapper function

we should defer machinery for extensibility until not having that machinery is causes grief.

also the second any of root appears to be never used, we can likely drop.

If a throttle utility is to be used, we should do so to avoid extra bytes. I believe something like the following could be used:

function throttle(cb) { let cookie; return () => { clearTimeout(cookie); cookie = setTimeout(cb, throttleDelay) // anything else, maybe the dirty check thing. } } addEventListener('resize', throttle(windowSetDimensionsMeta)) addEventListener('scroll', throttle(windowSetScrollMeta));

lynchbomb · 2018-02-23T19:02:46Z

@asakusuma

Does this change handle nested roots?

It does not. I think at first pass we should focus on MVP. Especially since we are back-porting root functionality and the ideal is that we roll a new release of Spaniel. In-which at that point we can include nested roots.

I assume this handles the scenario where some other code captures the window scroll event?

Nice catch actually. It does not. I'll include a fallback for the window and root scroll events to leverage page offset and clientRect deltas respectively. Since we don't want to check clientRect per frame as the primary identifier for isDirty validation when stale; as this is the performance anti-pattern we are trying to remove.

stefanpenner · 2018-02-27T23:46:03Z

src/metal/scheduler.ts

      this.engine = getGlobalEngine();
    }
+
+    if (root) {


I think we can drop this guard, setting root regardless will ensure the schedular will have the same number of slots between different instances. In addition, we do use this.root, and thus is may as well be set on at this level, rather then falling back down the prototype chain.

stefanpenner · 2018-02-27T23:56:16Z

src/metal/window-proxy.ts

  getHeight: nop,
  getWidth: nop,
+  rootsList: [],
  onWindowIsDirtyListeners: [],


This data-structure seems better suited to be a Map then an Array.

stefanpenner · 2018-02-27T23:56:45Z

src/metal/window-proxy.ts

+  rootsList: [],
  onWindowIsDirtyListeners: [],
  rAF: hasRAF ? window.requestAnimationFrame.bind(window) : (callback: Function) => { callback(); },
+  bindRootScrollEvent: (root: Element) => { root.addEventListener('scroll', () => W.callIsDirtyListeners(), false); },


what disconnects these listeners from custom roots?

stefanpenner · 2018-02-27T23:58:00Z

src/metal/window-proxy.ts

  rAF: hasRAF ? window.requestAnimationFrame.bind(window) : (callback: Function) => { callback(); },
+  bindRootScrollEvent: (root: Element) => { root.addEventListener('scroll', () => W.callIsDirtyListeners(), false); },
+  callIsDirtyListeners() {
+    if (this.onWindowIsDirtyListeners.length <= 0) { return; }


This guard seems redundant, as forEach wont iterate if the list is empty. We should likely only add more code (for perf) if it we have seen a good reason to do so.

stefanpenner · 2018-02-28T00:00:00Z

src/metal/window-proxy.ts

  },
-  __destroy__() {
+  disconnectIsDirtyListener(id: string) {
+    this.onWindowIsDirtyListeners = this.onWindowIsDirtyListeners.filter((obj: any) => {


if this becomes a map, this would become: this.onWindowIsDirtyListeners.delete(id)

if we do the above VERSION + instance._lastVersion stuff, this can be removed entirely.

stefanpenner · 2018-02-28T00:35:27Z

src/native-watcher.ts

+    if (root) {
+      if (!W.rootsList.includes(root)) {
+        W.rootsList.push(root);
+        W.bindRootScrollEvent(root);


what unbinds this scroll event

stefanpenner · 2018-02-28T00:35:53Z

src/native-watcher.ts


+    if (root) {
+      if (!W.rootsList.includes(root)) {
+        W.rootsList.push(root);


what removes from this list

stefanpenner · 2018-02-28T00:37:29Z

src/utils.ts

-    return element.getBoundingClientRect();
+    return element.getBoundingClientRect() as SpanielClientRectInterface;
  } catch (e) {
    if (typeof e === 'object' && e !== null && e.description.split(' ')[0] === 'Unspecified' && (e.number & 0xFFFF) === 16389) {


oh IE, you so cray cray

would it be safe to drop e.description.split(' ')[0] === 'Unspecified' && ?

stefanpenner · 2018-02-28T00:38:35Z

src/watcher.ts


+    if (root) {
+      if (!W.rootsList.includes(root)) {
+        W.rootsList.push(root);


what removes from this list?

stefanpenner · 2018-02-28T00:38:42Z

src/watcher.ts

+    if (root) {
+      if (!W.rootsList.includes(root)) {
+        W.rootsList.push(root);
+        W.bindRootScrollEvent(root);


what unbinds this event?

stefanpenner · 2018-02-28T00:44:41Z

src/metal/scheduler.ts

  ) {}
-  static generate(): Frame {
+  static generate(root?: Element): Frame {
+    if (root && document.documentElement.contains(root)) {


This guard, and the guard contained with getBoundingClientRect may be redundant when used together. As if the getBoundingClientRect fails, the various unknown attributes become 0,0,0,0 already right?

lynchbomb · 2018-03-01T18:46:41Z

@stefanpenner and I spoke at length offline yesterday, about a hypothetical alternative to the reviewed comments and PR implementation above. In-which the hypothesis will need to be tested. If proven to be a viable alternative. The release of these changes must be behind a minor release + flag. This includes but not limited to:

Removing the window and root listeners and instead having consumers of Spaniel provide a propagated "virtual event" on state change. For example scroll and resize events as well as state mutations on reads/writes. The implementation of this will be more or less a propagated isDirty boolean, passed directly to the Spaniel#Watcher instance. Spaniel will hook into this "virtual event" to invalidate the cached getBoundingClientRect of the window, root and/or ElementScheduler watched Element.
Since root is now always passed (either as the default window or root element) safety checks can be removed as well as conditionals for isRoot.

The above implementation and API change would also set us up for major simplifications. Which would be included within a future v4 major release. Such as:

A hard engine dependency on Backburner
Removal of Scheduler#tick
Removal of Engine

lynchbomb · 2018-03-22T23:05:15Z

@stefanpenner FWIW here is the PR spike for Ember-Spaniel implementation of this PR. Most importantly onInViewportOnce() on a custom root as well as revalidate() for forced cached validation on getBoundingClientRect of the root and scheduler as well as window meta.

lynchbomb · 2018-03-28T16:40:39Z

I chatted with @krisselden offline regarding the review of this PR. The primary feedback was when a custom root element is being watched, we should bind a scroll event within Spaniel on that custom root element to trigger cached state validation (ie the first iteration of this PR). That the common case would always require a scroll event on the custom root; keeping the API as dead simple as possible. Additionally (4/19/18):

Leveraging the Engine read/write within the WindowProxy when updating metadata.
Modify the WindowProxy to follow the same state invalidation pattern as the scheduler.
forceStateValidation should only check version NOT read/cache window metadata. Possibly rename to invalidateState.
If possible don't entangle the root and elements within ElementScheduler.
If possible discourage consumers from leveraging forceStateValidation everywhere

Additionally and outside the scope of this PR about the idea of Backburner implementing a measure queue, in-which would be highly leveragable with Spaniel.

krisselden · 2018-05-02T17:56:04Z

src/metal/window-proxy.ts

  }
 };

+export function validateState() {


name is confusing, I expected invalidate() or incrementVersion() or makeStateDirty()

krisselden · 2018-05-02T17:58:22Z

src/metal/window-proxy.ts

+  version: number;
+  lastVersion: number;
+  updateMeta: Function;
+  isDirty: boolean;


why have an isDirty + version + lastVersion?

isDirty is doing the validation check from version to lastVersion.

For this PR I am not going to address this. However we will table this and include in Spaniel 4.0.0:

Cached version state on the Window Proxy will be moved to have its own interface with a version.

One flat object with the prefix last that pertain to the lastVersion ie lastWidth,
lastTop etc.

krisselden · 2018-05-02T21:14:03Z

test/app/index.js

+  ALLOW_CACHED_SCHEDULER: true
+});
+
+root.addEventListener('scroll', () => spaniel.invalidate(), false);


this doesn't look like it is removed if the observer is disposed

I realize this is a test, but is this the pattern that is encouraged?

Just as the consuming app will need to handle binding the event they are also responsible for removing it. In Ember-Spaniel for example, we will provide a clear API to do both (on/off) within Embers lifecycle hooks.

lynchbomb requested review from asakusuma and krisselden February 20, 2018 23:11

lynchbomb mentioned this pull request Feb 22, 2018

Scheduler and window proxy listeners state. Client Rect Poly for IE 11 #79

Merged

asakusuma reviewed Feb 23, 2018

View reviewed changes

stefanpenner suggested changes Feb 28, 2018

View reviewed changes

lynchbomb force-pushed the lynchbomb branch 3 times, most recently from d8b8c9b to 190bcc3 Compare March 19, 2018 22:11

lynchbomb mentioned this pull request Mar 22, 2018

New API methods including invalidate and custom roots ember-spaniel/ember-spaniel#25

Merged

lynchbomb force-pushed the lynchbomb branch 2 times, most recently from 9959766 to b49d3b2 Compare April 23, 2018 16:58

lynchbomb force-pushed the lynchbomb branch 2 times, most recently from 992951c to 1387e54 Compare May 2, 2018 17:47

krisselden reviewed May 2, 2018

View reviewed changes

backporting the root implementation with memoize feature

664a91b

lynchbomb force-pushed the lynchbomb branch from 1387e54 to 664a91b Compare May 2, 2018 20:59

krisselden reviewed May 2, 2018

View reviewed changes

lynchbomb merged commit f79345b into v2-release May 3, 2018

lynchbomb deleted the lynchbomb branch May 3, 2018 17:36

Conversation

lynchbomb commented Feb 20, 2018

Uh oh!

asakusuma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stefanpenner Feb 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lynchbomb commented Feb 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lynchbomb commented Mar 1, 2018

Uh oh!

lynchbomb commented Mar 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lynchbomb commented Mar 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lynchbomb May 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

stefanpenner Feb 28, 2018 •

edited

Loading

lynchbomb commented Feb 23, 2018 •

edited

Loading

lynchbomb commented Mar 22, 2018 •

edited

Loading

lynchbomb commented Mar 28, 2018 •

edited

Loading

lynchbomb May 2, 2018 •

edited

Loading