misc(build): fix mangling for tap-targets gatherer

[connorjclark]

When minified for CDT, the tap-targets gatherer fails. yarn test-devtools didn't catch it because there is no layout test that exercise that particular code path in the page. Running it on other pages showed the problem. (yarn open-devtools is so useful)

overloading variables between Node/page-function contexts is bad, it causes tooling to make mistakes.

[connorjclark]

This is how all other gatherers import this module. Only this one did destructuring, which results in the reference to getRectCenterPoint in the "page function" below to (lexically) check out. but lexical scope is a damned lie here

[brendankenny]

Ah, so I found this confusing, because obviously it has to be in scope to be able to toString() it, at which point it'll also be in scope on the browser side. The issue is the combo with keep_fnames: true.

• Destructuring ends up looking something like {getRectCenterPoint: o} = e("../../../lib/rect-helpers.js") since getRectCenterPoint wasn't renamed as a function, but as a local variable terser thinks it can be renamed.

• o.toString gives a function with the original name ('function getRectCenterPoint() {...}'), though, so when code tries to call o() it throws an error

Ideally we could eventually go the opposite way of this, having the toString() and the function calls match, as that would let us do keep_fnames: false and save 30KiB.

[patrickhulce]

can we add a smoketest to test-bundle that ensures no audits throw an exception? seems like it would've been good to not have to manually catch this (and for future changes to bundlification)

[connorjclark]

can we add a smoketest to test-bundle that ensures no audits throw an exception?

I'm confused–our smoke testing doesn't support this. What do you mean?

We could just run all the smoke tests instead of the subset we do. If just one of them asserts on tap-target items, it would have caught this.

[patrickhulce]

I'm confused–our smoke testing doesn't support this. What do you mean?

Sure it does, scoreDisplayMode: /informative|binary|numeric|notApplicable/ :) but ya it wouldn't be fun to add

We could just run all the smoke tests instead of the subset we do. If just one of them asserts on tap-target items, it would have caught this.

That works too. Just seems important to increase test coverage of our bundle as we increasingly do more things to it

[Beytoven]

Nit: Can we remove these unused RectHelpers functions? Looks like only getRectCenterPoint() is used so the others aren't needed.

[connorjclark]

nice catch

[brendankenny]

Just seems important to increase test coverage of our bundle as we increasingly do more things to it

This seems really important, since it's not going to be obvious that writing normal JS might break when minified (same for the fix needed in d657e76).

Since we're going all in on minification, it does seem like turning on all smoke tests for the bundle is necessary now.

The tap-targets gatherer was always failing in CDT

But not caught in the devtools smoke tests, right?

[connorjclark]

Just seems important to increase test coverage of our bundle as we increasingly do more things to it

This seems really important, since it's not going to be obvious that writing normal JS might break when minified (same for the fix needed in d657e76).

I'll open a PR now that runs every smoke test on the bundle.

Since we're going all in on minification, it does seem like turning on all smoke tests for the bundle is necessary now.

The tap-targets gatherer was always failing in CDT

But not caught in the devtools smoke tests, right?

Sorry, that was very vague, I updated the first comment.