Proposal: add inline assertions that run only in a new debug build

brendankenny commented 2 weeks ago

The problem

There are a few tricky areas within web-vitals where it's difficult to be certain that they're always correct because they're written to:

run quickly
not bloat the library size
run from inside the polyfilled and monkeypatched fun house that is any give web page
default to silently handle apparent impossibilities so a web-vitals exception doesn't take down the whole page

Testing is often difficult because failures tend to only occur in very unusual cases, but we have to know about the unusual case failures to test them, and web-vitals is written to not fall over in those unusual cases. Failures that do get isolated may still be flaky and difficult to reproduce, depending on races between browser scheduling and PerformanceEntry emitting (two recent examples: #536, #496).

Asserting the problem

Adding a debug build with assertions inline in the code could be a powerful tool to help with this. A quick example branch of what this could look like: https://github.com/GoogleChrome/web-vitals/compare/f6290e9...debug-assertions

The assertions would look like some permutation of

// pendingLoAFs must be in order of startTime.
if (DEBUG) {
  for (let i = 1; i < pendingLoAFs.length; i++) {
    if (pendingLoAFs[i].startTime < pendingLoAFs[i - 1].startTime) {
      throw new Error('pendingLoAFs not sorted by timestamps');
    }
  }
}

or pull the assertion out somewhere else

import {assertArrayInOrder} from './assertions.js';
// ...
if (DEBUG) {
  assertArrayInOrder(pendingLoAFs.map(l => l.startTime));
}

Rollup/terser can replace DEBUG with a constant false in the normal build and the entire code block will be dropped, resulting in no extra bytes shipped and no new possible errors for users (try out the branch to see it in action).

I believe the DEBUG check will also always work inside the assertion function as long as the if statement is the only thing at the top level, but I don't want to promise that yet if I'm wrong and terser sometimes misses that the resulting function is a no-op.

Meanwhile, debug builds run the assertions so they complain loudly when run in tests and we can periodically deploy to some relatively popular site, with errors reported to analytics. Users reporting issues can also be asked to deploy a debug build (in the style of https://github.com/GoogleChrome/web-vitals/issues/496#issuecomment-2166593956) to help gather data on how the error is occurring. Bonus: all the infrastructure is there if you want to add a bunch more assertions for your pet bug that day so you can make your own debug build and try it out on some test page.

I think we'd have to be disciplined to not have these everywhere—we don't want to be double checking even a small number of property accesses—but as a first pass at rules of thumb:

Any time we have an invariant that has to be true for other code to function correctly, add an assertion to ensure that it is true (e.g. pending LoAFs have to be in startTime order, event entries are emitted in startTime order, etc)
Whenever we get a user report that something's erroring, fix the probable bug defaulting to silently skipping the erroneous state, but also add an assertion against it (e.g. cannot read properties of undefined -> assert the object is present so we can start seeing in what circumstances it can become undefined)

To fight against entropy, we could also require a plan when adding assertions, e.g. this one will live forever to help prevent future bugs (e.g. events stay sorted), this one can eventually be retired now that we've determined the case where it errored and we now handle it.

Possible problems with asserting the problem

reading code gets noisier - there's more iterating we could do on the examples from above if they look ugly, but we have to be sure to fit into terser's model for total dead code elimination to not have extra bytes. As mentioned above, the DEBUG check could go inside the assertion function, but we'd have to make sure terser was then inlining the empty function body and doesn't ever keep the function call around. You might still run into simplicity issues when you're asserting on some convoluted mapping over an array, or you want a more specific error message than just expected PerformanceEntry but got undefined or whatever.
anyone importing individual web-vitals scripts will get a ReferenceError thrown on the DEBUG check - Unfortunately the way terser conditional compilation works, the variable has to be a bare global variable (you can't do globalThis.DEBUG or similar). It's easy to default to false, most other build tools support this (e.g. esbuild, closure, etc), and at worst you could always do a regex replace, but you have to know to set that up in the first place. I'm not sure how big a drawback this is. Current ideas: could use a more identifiable variable name so you know for certain where to look for answers? DEBUG_WEB_VITALS or something instead of plain DEBUG. we'll replace the DEBUG variable in both the bundles and individual files published to npm so no one will run into this
There's a risk of extra shipped bytes if we're not paying attention. e.g.
```
const debugData = aBunchOfExtraWorkJustForAsserting();
if (DEBUG) {
 assert(debugData);
}
```
ideas: set up the CI actions to print a before/after byte count for every PR. The extra bytes might still disappear in any PR where code is being added in addition to assertions, but it would help, and kind of feels like we should have this anyways on a byte-conscious project.

Thoughts?

tunetheweb commented 2 weeks ago

anyone importing individual web-vitals scripts will get a ReferenceError thrown on the DEBUG check

That's gonna be a big problem IMHO as we have lots of people doing this internally at Google at least.

brendankenny commented 2 weeks ago

anyone importing individual web-vitals scripts will get a ReferenceError thrown on the DEBUG check

That's gonna be a big problem IMHO as we have lots of people doing this internally at Google at least.

As discussed out of band, the Google case is the easy one due to orderly build rule interfaces between libraries.

I think you're right that leaving DEBUG isn't a great solution for library users though, and we should eat the cost.

New proposal: The current individual imports get created by tsc and put in dist/modules/. It would be pretty easy to transform those files in place, swapping the debug variable for the expected boolean constant. Users would still need their own minifier to eliminate the dead code blocks in the false case, but that's not any different than what they already have to do to minify comments, whitespace, variable names, etc.

Terser would then run on those files to make the bundles, no terser global_defs required.

GoogleChrome / web-vitals