Defer CLS logic until after onFCP callback (#297)

* Defer CLS logic until after onFCP callback * Address review feedback
GoogleChrome · Jan 9, 2023 · e5e4a6d · e5e4a6d
1 parent cd560dc
commit e5e4a6d
Show file tree

Hide file tree

Showing 24 changed files with 467 additions and 212 deletions.
diff --git a/.eslintrc b/.eslintrc
@@ -12,9 +12,7 @@
   "overrides": [
     {
       "files": "wdio.conf.js",
-      "extends": [
-        "eslint:recommended", "google"
-      ],
+      "extends": ["eslint:recommended"],
       "rules": {
         "max-len": "off"
       }
@@ -25,61 +23,72 @@
         "$": false,
         "browser": false
       },
-      "extends": [
-        "eslint:recommended", "google"
-      ],
+      "extends": ["eslint:recommended"],
       "rules": {
-        "comma-dangle": ["error", {
-          "arrays": "always-multiline",
-          "objects": "always-multiline",
-          "imports": "always-multiline",
-          "exports": "always-multiline",
-          "functions": "never"
-        }],
+        "comma-dangle": [
+          "error",
+          {
+            "arrays": "always-multiline",
+            "objects": "always-multiline",
+            "imports": "always-multiline",
+            "exports": "always-multiline",
+            "functions": "never"
+          }
+        ],
         "indent": ["error", 2],
         "no-invalid-this": "off",
-        "max-len": [2, {
-          "ignorePattern": "^\\s*import|= require\\(|^\\s*it\\(|^\\s*describe\\(",
-          "ignoreUrls": true
-        }],
-        "space-before-function-paren": ["error", {
+        "max-len": [
+          2,
+          {
+            "ignorePattern": "^\\s*import|= require\\(|^\\s*it\\(|^\\s*describe\\(",
+            "ignoreUrls": true
+          }
+        ],
+        "space-before-function-paren": [
+          "error",
+          {
             "anonymous": "always",
             "named": "never",
             "asyncArrow": "always"
-        }]
+          }
+        ]
       }
     },
     {
       "files": "src/**/*.ts",
       "parser": "@typescript-eslint/parser",
-      "extends": [
-        "plugin:@typescript-eslint/recommended"
-      ],
+      "extends": ["plugin:@typescript-eslint/recommended"],
       "rules": {
         "@typescript-eslint/no-non-null-assertion": "off",
         "@typescript-eslint/no-use-before-define": "off",
         "@typescript-eslint/explicit-function-return-type": "off",
         "@typescript-eslint/explicit-module-boundary-types": "off",
         "@typescript-eslint/ban-ts-comment": "off",
         "@typescript-eslint/camelcase": "off",
-        "comma-dangle": ["error", {
-          "arrays": "always-multiline",
-          "objects": "always-multiline",
-          "imports": "always-multiline",
-          "exports": "always-multiline",
-          "functions": "never"
-        }],
+        "comma-dangle": [
+          "error",
+          {
+            "arrays": "always-multiline",
+            "objects": "always-multiline",
+            "imports": "always-multiline",
+            "exports": "always-multiline",
+            "functions": "never"
+          }
+        ],
         "indent": ["error", 2],
         "node/no-missing-import": "off",
         "node/no-unsupported-features/es-syntax": "off",
         "node/no-missing-require": "off",
         "node/shebang": "off",
         "no-dupe-class-members": "off",
-        "space-before-function-paren": ["error", {
+        "space-before-function-paren": [
+          "error",
+          {
             "anonymous": "always",
             "named": "never",
             "asyncArrow": "always"
-        }]
+          }
+        ]
       },
       "parserOptions": {
         "ecmaVersion": 2018,

diff --git a/.husky/.gitignore b/.husky/.gitignore
@@ -0,0 +1 @@
+_
diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -164,7 +164,6 @@
     "body-parser": "^1.20.0",
     "chromedriver": "^107.0.3",
     "eslint": "^8.24.0",
-    "eslint-config-google": "^0.14.0",
     "express": "^4.18.1",
     "fs-extra": "^10.1.0",
     "husky": "^8.0.1",

diff --git a/src/lib/onHidden.ts b/src/lib/onHidden.ts
@@ -18,14 +18,10 @@ export interface OnHiddenCallback {
   (event: Event): void;
 }
 
-export const onHidden = (cb: OnHiddenCallback, once?: boolean) => {
+export const onHidden = (cb: OnHiddenCallback) => {
   const onHiddenOrPageHide = (event: Event) => {
     if (event.type === 'pagehide' || document.visibilityState === 'hidden') {
       cb(event);
-      if (once) {
-        removeEventListener('visibilitychange', onHiddenOrPageHide, true);
-        removeEventListener('pagehide', onHiddenOrPageHide, true);
-      }
     }
   };
   addEventListener('visibilitychange', onHiddenOrPageHide, true);

diff --git a/src/lib/runOnce.ts b/src/lib/runOnce.ts
@@ -0,0 +1,29 @@
+/*
+ * Copyright 2022 Google LLC
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+export interface RunOnceCallback {
+  (arg: unknown): void;
+}
+
+export const runOnce = (cb: RunOnceCallback) => {
+  let called = false;
+  return (arg: unknown) => {
+    if (!called) {
+      cb(arg);
+      called = true;
+    }
+  };
+};
diff --git a/src/onCLS.ts b/src/onCLS.ts
@@ -17,10 +17,10 @@
 import {onBFCacheRestore} from './lib/bfcache.js';
 import {initMetric} from './lib/initMetric.js';
 import {observe} from './lib/observe.js';
-import {onHidden} from './lib/onHidden.js';
 import {bindReporter} from './lib/bindReporter.js';
 import {doubleRAF} from './lib/doubleRAF.js';
-import {whenActivated} from './lib/whenActivated.js';
+import {onHidden} from './lib/onHidden.js';
+import {runOnce} from './lib/runOnce.js';
 import {onFCP} from './onFCP.js';
 import {CLSMetric, CLSReportCallback, ReportOpts} from './types.js';
 
@@ -49,99 +49,88 @@ export const onCLS = (onReport: CLSReportCallback, opts?: ReportOpts) => {
   // Set defaults
   opts = opts || {};
 
-  whenActivated(() => {
-    // https://web.dev/cls/#what-is-a-good-cls-score
-    const thresholds = [0.1, 0.25];
+  // Start monitoring FCP so we can only report CLS if FCP is also reported.
+  // Note: this is done to match the current behavior of CrUX.
+  onFCP(
+    runOnce(() => {
+      // https://web.dev/cls/#what-is-a-good-cls-score
+      const thresholds = [0.1, 0.25];
 
-    let metric = initMetric('CLS');
-    let report: ReturnType<typeof bindReporter>;
+      let metric = initMetric('CLS', 0);
+      let report: ReturnType<typeof bindReporter>;
 
-    let fcpValue = -1;
-    let sessionValue = 0;
-    let sessionEntries: PerformanceEntry[] = [];
-
-    const onReportWrapped: CLSReportCallback = (arg) => {
-      if (fcpValue > -1) {
-        onReport(arg);
-      }
-    };
+      let sessionValue = 0;
+      let sessionEntries: PerformanceEntry[] = [];
 
-    // const handleEntries = (entries: Metric['entries']) => {
-    const handleEntries = (entries: LayoutShift[]) => {
-      entries.forEach((entry) => {
-        // Only count layout shifts without recent user input.
-        if (!entry.hadRecentInput) {
-          const firstSessionEntry = sessionEntries[0];
-          const lastSessionEntry = sessionEntries[sessionEntries.length - 1];
+      // const handleEntries = (entries: Metric['entries']) => {
+      const handleEntries = (entries: LayoutShift[]) => {
+        entries.forEach((entry) => {
+          // Only count layout shifts without recent user input.
+          if (!entry.hadRecentInput) {
+            const firstSessionEntry = sessionEntries[0];
+            const lastSessionEntry = sessionEntries[sessionEntries.length - 1];
 
-          // If the entry occurred less than 1 second after the previous entry
-          // and less than 5 seconds after the first entry in the session,
-          // include the entry in the current session. Otherwise, start a new
-          // session.
-          if (
-            sessionValue &&
-            entry.startTime - lastSessionEntry.startTime < 1000 &&
-            entry.startTime - firstSessionEntry.startTime < 5000
-          ) {
-            sessionValue += entry.value;
-            sessionEntries.push(entry);
-          } else {
-            sessionValue = entry.value;
-            sessionEntries = [entry];
+            // If the entry occurred less than 1 second after the previous entry
+            // and less than 5 seconds after the first entry in the session,
+            // include the entry in the current session. Otherwise, start a new
+            // session.
+            if (
+              sessionValue &&
+              entry.startTime - lastSessionEntry.startTime < 1000 &&
+              entry.startTime - firstSessionEntry.startTime < 5000
+            ) {
+              sessionValue += entry.value;
+              sessionEntries.push(entry);
+            } else {
+              sessionValue = entry.value;
+              sessionEntries = [entry];
+            }
           }
+        });
 
-          // If the current session value is larger than the current CLS value,
-          // update CLS and the entries contributing to it.
-          if (sessionValue > metric.value) {
-            metric.value = sessionValue;
-            metric.entries = sessionEntries;
-            report();
-          }
-        }
-      });
-    };
-
-    const po = observe('layout-shift', handleEntries);
-    if (po) {
-      report = bindReporter(
-        onReportWrapped,
-        metric,
-        thresholds,
-        opts!.reportAllChanges
-      );
-
-      // Start monitoring FCP so we can only report CLS if FCP is also reported.
-      // Note: this is done to match the current behavior of CrUX.
-      // Also, if there have not been any layout shifts when FCP is dispatched,
-      // call "report" with a zero value
-      onFCP((fcpMetric) => {
-        fcpValue = fcpMetric.value;
-        if (metric.value < 0) {
-          metric.value = 0;
+        // If the current session value is larger than the current CLS value,
+        // update CLS and the entries contributing to it.
+        if (sessionValue > metric.value) {
+          metric.value = sessionValue;
+          metric.entries = sessionEntries;
           report();
         }
-      });
-
-      onHidden(() => {
-        handleEntries(po.takeRecords() as CLSMetric['entries']);
-        report(true);
-      });
+      };
 
-      // Only report after a bfcache restore if the `PerformanceObserver`
-      // successfully registered.
-      onBFCacheRestore(() => {
-        sessionValue = 0;
-        fcpValue = -1;
-        metric = initMetric('CLS', 0);
+      const po = observe('layout-shift', handleEntries);
+      if (po) {
         report = bindReporter(
-          onReportWrapped,
+          onReport,
           metric,
           thresholds,
           opts!.reportAllChanges
         );
 
-        doubleRAF(() => report());
-      });
-    }
-  });
+        onHidden(() => {
+          handleEntries(po.takeRecords() as CLSMetric['entries']);
+          report(true);
+        });
+
+        // Only report after a bfcache restore if the `PerformanceObserver`
+        // successfully registered.
+        onBFCacheRestore(() => {
+          sessionValue = 0;
+          metric = initMetric('CLS', 0);
+          report = bindReporter(
+            onReport,
+            metric,
+            thresholds,
+            opts!.reportAllChanges
+          );
+
+          doubleRAF(() => report());
+        });
+
+        // Queue a task to report (if nothing else triggers a report first).
+        // This allows CLS to be reported as soon as FCP fires when
+        // `reportAllChanges` is true.
+        setTimeout(report, 0);
+      }
+    })
+  );
 };