refactor: performance timers for node.js and browser

[dnalborczyk]

This PR contains:

• bugfix
• feature
• refactor
• documentation
• other

Are tests included?

• yes (bugfixes and features will not be merged without tests)
• no

Breaking Changes?

• yes (breaking changes will not be merged unless absolutely necessary)
• no

List any relevant issue numbers:

Description

distinguishes between bundled files for node.js and browsers (alternative envs). replaces process.hrtime with perf_hooks/globalThis.performance

[codecov]

Codecov Report

Merging #4340 (22cc2c5) into master (edca64e) will increase coverage by 0.07%.
The diff coverage is 74.07%.

@@            Coverage Diff             @@
##           master    #4340      +/-   ##
==========================================
+ Coverage   98.43%   98.51%   +0.07%     
==========================================
  Files         206      206              
  Lines        7344     7333      -11     
  Branches     2088     2084       -4     
==========================================
- Hits         7229     7224       -5     
+ Misses         55       51       -4     
+ Partials       60       58       -2     

Impacted Files	Coverage Δ
src/utils/crypto.ts	100.00% <ø> (ø)
src/utils/timers.ts	58.92% <72.00%> (+2.21%)	⬆️
src/utils/collapseSourcemaps.ts	90.21% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update edca64e...22cc2c5. Read the comment docs.

[kzc]

Ignore this comment if it's a known issue with this Draft PR.

I think a level of indirection is needed here - via typeof or function. Although this would work in a recent browser, referencing the global performance at top level causes the browser bundle and its tests to fail on node <= v14:

$ node-v10.4.1 -e performance 2>/dev/null && echo OK || echo FAIL
FAIL
$ node-v12.13.0 -e performance 2>/dev/null && echo OK || echo FAIL
FAIL
$ node-v14.16.0 -e performance 2>/dev/null && echo OK || echo FAIL
FAIL
$ node-v16.1.0 -e performance 2>/dev/null && echo OK || echo FAIL
OK

[dnalborczyk]

yeah, ideally the global performance object would be used for both browser and node.js. I was under the impression it was supported as a global in v14, but you are right, it's only available in v16, unless someone would do a backport 😞

if I understood the bundle process correctly, the file used for the node.js build would be this one: https://github.com/rollup/rollup/pull/4340/files#diff-56aca2bfe43f23ab215af1f0c17d5ac2abbed4deb560487408bc588c47c2fc44R1 being replaced with this call:

rollup/rollup.config.ts

Line 160 in da3bb43

replaceBrowserModules(),

originally I wrapped and exported the now function only, but I was thinking it's probably better to make the entire performance object (for future use) available. I was also thinking to remove the timer Map, and use performance.measure and/or performance observer instead. but it seems currently it wouldn't be possible to attach any other metadata, e.g. for the value for the memory consumption. (detail param was added in v16)

[dnalborczyk]

ah, I see. did you mean this: https://github.com/rollup/rollup/runs/4723627853?check_suite_focus=true#step:7:5615 just noticed. I have to check it out. I suppose those tests are not running in any browser but in node.js 🤔

[kzc]

Yeah, the browser bundle name is a bit of a misnomer. It really means JS platform independent.

[dnalborczyk]

@lukastaegert I was wondering if this is a bug: I added perf_hooks to externals, but noticed that e.g. removing any of the items, or even removing (commenting out) all externals makes no difference. the expected warnings seem to be missing.

[lukastaegert]

Looks good to me!

[lukastaegert]

I was wondering if this is a bug: I added perf_hooks to externals, but noticed that e.g. removing any of the items, or even removing (commenting out) all externals makes no difference. the expected warnings seem to be missing.

I think the reason is that since some versions, @rollup/plugin-node-resolve is automatically marking node builtins as external. Looking at the list, I think we only really need fsevents in here.

[dnalborczyk]

I think the reason is that since some versions, @rollup/plugin-node-resolve is automatically marking node builtins as external. Looking at the list, I think we only really need fsevents in here.

ah, gotcha, makes sense. I'll put up another PR to remove those.