Fallback resource timing in web workers

[sbma44]

This PR adds a fallback method to collectResourceTiming: true for browsers that don't implement the performance.getEntries() method properly in web workers. It provides as much data as can be collected now, and should return full data when these browsers correct their implementations.

The availability of the performance API in web workers remains unpleasantly murky. Despite tickets like https://bugzilla.mozilla.org/show_bug.cgi?id=1425458 and badges declaring web worker availability bugs remain in major browsers. Specifically, it appears that in both Firefox and Safari performance.getEntries, performance.getEntriesByName and performance.getEntriesByType return empty arrays in web workers regardless of whether the worker has made network requests.

performance.mark, performance.measure and subsequent use of performance.getEntries* do work, however. This is an inferior solution, as PerformanceMeasure objects compare unfavorably to PerformanceResourceTiming objects:

• no DNS, TLS or connection timing data
• no division between request and response
• no data on payload size

Still, the basic timing data that can be collected is better than nothing.

This change does not alter any codepaths when collectResourceTiming is false. When it is true, a check on array length and an invocation of performance.mark() are added to the status quo codepath. Obviously there's a bit more code for the fallback case, but it's pretty slight.

Launch Checklist

• briefly describe the changes in this PR
• write tests for all new functionality
• document any changes to public APIs n/a
• post benchmark scores
• manually test the debug page tested on my instrumentile integration test page instead (GL JS debug page doesn't collect resource timing)

[sbma44]

Please let me know if you'd like to see benchmarks here -- I haven't bothered running them yet given how slight this change is.

[jfirebaugh]

Can you DRY up the repeated code here / extract it to util/performance.js somehow?

[jfirebaugh]

What's the rationale for this change? Doesn't look like bcryptjs is in the dependency tree.

[ryanhamley]

I ran into something similar to this the other day when I was using npm install rather than yarn install. I can't say for sure that's where this was introduced but I kept getting a test error in bcrypt until I removed node modules and reinstalled with yarn. This shouldn't be needed.

[sbma44]

It's present in my node_modules after a fresh npm i and causes flow test failures thanks to some templating wackiness. Not related to this PR and I'm happy to do whatever's most appropriate here. Replicated with node 6 and 8.

[ryanhamley]

Tom, if you remove this line from .flowconfig then rm -rf node_modules and reinstall with yarn install does the bcrypt error go away?

[sbma44]

Ah, posted before refreshing to see @ryanhamley's comment. I see that yarn is the default here, if npm support isn't a consideration I'm happy to remove this change.

[ryanhamley]

I think npm install should work the way you expected it to. We need to debug why it isn't, but that's a separate issue.

[jfirebaugh]

npm install doesn't use yarn.lock, so it installs a newer version of @mapbox/batfish, which pulls in bcryptjs via some transitive dependencies.

Let's revert here and consider using https://www.npmjs.com/package/use-yarn to avoid this gotcha in the future.

[sbma44]

@ryanhamley yes, confirmed that using yarn avoids the problem

@jfirebaugh sounds good, I've removed the relevant commit

[ryanhamley]

I opened a ticket for the npm bug #6774

[sbma44]

Rebasing away the bower config commit did something weird to my last merge from master, prompting another merge and a strange-looking commit history in this PR. LMK if this is an issue and you need a fresh branch.