ref(utils): Improve uuid generation

[timfish]

Since modern browsers support crypto.randomUUID() it's probably a good idea to use that when available.
Rather than let this impact bundle size, I also took some of the recent updates from the linked stackoverflow question.

• Shaves ~160 bytes off the browser bundle
• I have validated that all code paths do in fact return valid uuidv4 ids with hyphens removed!
• Modern platforms bail out quickly with crypto.randomUUID()
• Less modern browsers (including IE11 and Safari < v15.4) use crypto.getRandomValues()
• Node.js
  • < v15 uses Math.random()
  • v15 > v16.7 uses crypto.getRandomValues()
  • >= v16.7 uses crypto.randomUUID()

Downsides vs previous code:

• The "code golf" style code is nasty but since this is a utility function that will likely go untouched until it's replaced on all platforms with crypto.randomUUID(), this may be less of a concern. I added a comment for the most confusing number+string concat part.
• When using crypto.getRandomValues(), it's called multiple times rather than a single time with new Uint16Array(8) which could marginally affect performance.

[Lms24]

Looks like a good change to me. Took me a few minutes to figure out what's going on but I think in terms of readability, it's similar (maybe a little better) than before. Less bytes is always great and using crypto.randomUUID() whenever possible is a good change for sure.

Would you mind adding a test or two to make sure this function is covered?

[Lms24]

Awesome, thanks!

[timfish]

My only minor concern with the test is that we're not testing a version of node that hits getRandomValues

< v15 uses Math.random()
v15 > v16.7 uses crypto.getRandomValues()
>= v16.7 uses crypto.randomUUID()

[Lms24]

Right. Can we mock getGlobalObject to return an object that only contains crypto.getRandomValues but not crypto.randomUUID? Or does getRandomValues not exist anymore after 16.7?

[timfish]

A few things worth noting... although node supports getRandomValues and randomUUID on earlier versions, these are via the crypto module. Node didn't get the Web Crypto global API until v17.6. This means the versions actually get simplified to this:

• < v17.6 uses Math.random()
• >= v17.6 uses crypto.randomUUID()

A further spanner in the works of the tests is that Jest doesn't have the same globals as node.js or even the browser. This means that the crypto global is missing even in node v18 😭. I remember this same thing being an issue with global TextEncoder.

So for each uuid test, I construct the global crypto required to check that code path 🤷‍♂️

This does make me wonder whether Jest + node version test matrix gives us the coverage we'd be hoping for when globals are involved 🤔

[Lms24]

Thanks for updating the tests!
You're making a good point about the global object and Node versions. I'll bring it up with the team so that we're all aware of it.

[lobsterkatie]

I know this has already been merged, but just for the sake of context-sharing, since I did a bunch of work with our Jest setup earlier this year...

As for as Jest x Node versions goes, this has come up before in various ways. The problem is that we're at the mercy of a few intersecting issues:

• jest-environment-node and jest-environment-jsdom - the former mucks directly with global and the latter wraps jsdom, which by their own admission is missing some stuff

• Precisely what stuff jest-environment-node mucks with (and how) and what stuff jsdom is missing depends, naturally, on the version of each we're using. For us, versioning comes into play in two ways:

  • In oder to test on older versions of Node, we have to downgrade some of our testing packages, because the newer versions use syntax those old Node versions can't handle (bare catch is a frequent one here). When doing so, we try to downgrade little as possible.
  • In general, though, we're up to date with all of our versions (or were as of February or so).

  What both of those things mean is that there's no real correspondence between Node versions and Jest versions. (So, for example, a newer version of jest-environment-node might assume that something like TextEncoder is available globally when, for the version of Node we're using, it wouldn't have been.)

• In some cases (though not this one), we're testing code against older versions of Node when it's not necessary to do so, specifically any code which we know will only run in the browser. Node's the vehicle for those unit tests, but there's no requirement that @sentry/browser (and friends) be compatible with Node8/10/etc, even though we're testing them like there is.

In short, it's kind of a mess (albeit one which works decently well, even if not perfectly).

As far as amending what's available, in the future what you can do is create your own environment. Check out .jest/dom-environment.js in #3945, where I had to do so in order to make TextEncoder and TextDecoder available globally (which they are in browser but haven't always been in Node).

[timfish]

just for the sake of context-sharing

Thanks for this!

[bendehghan]

I'm having issues with this and Jest. I think the issue is with the UUID usage in jest as explained above. Have you had any luck with running tests in Jest?

[lobsterkatie]

We run all of our tests in Jest without any issue. What version of jest, jest-environment-node, and jest-environment-jsdom are you using, and what version of Node?