Support multiple calls to onINP() with different config options

[philipwalton]

Fixes #582.

This PR updates the interactions.ts logic to encapsulate it in a class that can be instantiated per onINP() call. It also moves most of the functions in the attribution/onINP.ts file inside of the onINP() closure, to keep all of the variables local to each onINP() call.

The main downside of this PR is it increases the bundle size of the library (since object properties don't minify as well as instance variables), but I added an additional terser config option to mangle object properties that begin with a $ character, which negates much of the increased size (at the expense of being a bit annoying to write).

[tunetheweb]

This all looks to work but I'm really, really, really hating the $ thing. I don't see how this is maintainable going forward. I'm already confused as to which variables should have a $ and which shouldn't (see comment below).

And this has just gets MORE complicated in #585 as we need to roll this pattern out to LCP and CLS :-(

There surely must be a better way to have separate entryPreProcessingCallbacks values (maybe keyed by a unique id created at each onINP instantiation?).

[tunetheweb]

Why are only two of these public, and one not? Seems weird to mix and match public and private names in the same interface.

[philipwalton]

Nit: don't think of the $ prefix and representing public or private (at least in the traditional sense of those words)—only whether or not the symbol will be mangled by terser. All of these symbols are "public" in the strict sense (in that they are accessible outside of their class), but "private" to the package (in that no consumer of the web-vitals library can access these methods).

As for why some of them have the $ prefix and some don't, the only reason they all don't is because the property names id, entries are already part of the public Metric API, and so the library compresses better if we don't mangle those symbols but instead reuse the existing dictionary entry for them.

We can probably add a comment in the code to avoid that confusion.

[tunetheweb]

Ahhh that's what I wasn't getting. Definitely needs a comment to explain this.

[brendankenny]

@philipwalton why $ and not _?

[tunetheweb]

_ is more common for internal variable names in my experience.

[philipwalton]

I originally had _, but then I was concerned that was confusing since a _ prefix typically implies private in the traditional sense (which this is not).

But I'm happy to move back to that if you'd all prefer it.

[tunetheweb]

Well _ is more a convention of "internal", than necessarily private. The newer # would strictly mean private. So I think _ is better.

[philipwalton]

Switched from $ to _.

[brendankenny]

Yeah, I don't feel strongly, but $ makes me think, "oh no, what weird thing are they doing with their build" while _ I think, "well, that's not exactly 'private', but close enough" :)

[tunetheweb]

Yup me too.

I also don’t think it needs documented as much with _ because it’s a more well-known pattern.

[philipwalton]

This all looks to work but I'm really, really, really hating the $ thing. I don't see how this is maintainable going forward. I'm already confused as to which variables should have a $ and which shouldn't (see comment below).

See my other comment for an explanation as to why some have the $ and some don't. The alternatives to using the $ prefix are:

• Default to compressing all property names but allowlist the ones we don't want to mangle in the config (which I think will be harder to maintain)
• Just live with the larger brotli size (which personally I don't want to do)

[tunetheweb]

With the switch to _ I feel more comfortable with this since it’s a more used pattern.