Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

uuid should not rely on module load order #536

Closed
drewthaler opened this issue Nov 20, 2020 · 6 comments · Fixed by statisticsnorway/ssb-component-library#256, Imageus-OSS/server#33, concourse/concourse#6684, #808 or #809

Comments

@drewthaler
Copy link
Contributor

Is your feature request related to a problem? Please describe.

uuid 7.x uses Crypto.getRandomValues which is not implemented in react-native, and attempts to work around that by asking clients to require another module — react-native-get-random-values — to implement a polyfill. It further caches the polyfill at module load time, requiring users to enforce module load order or else it breaks.

First, relying on module load order is fragile and prone to breakage. In particular, I've run into a problem with Expo where the expo-updates Updates.reloadAsync is reloading our app code in a situation where the uuid module is already initialized — probably because expo-updates uses it.

Second, this causes confusion among developers. At present it's ~10% of the last 32 github issues. (#531, #514, #499).

Describe the solution you'd like

Multiple possible solutions could be implemented. They are not mutually exclusive.

  1. uuid could directly rather than indirectly depend on this module that it certainly depends on, importing a symbol and using that, rather than hoping the polyfill gets installed in time.
  2. uuid could fold a minimal version of the polyfill code (which is rather simple) into the module and use that as a fallback
  3. uuid could lazy-load the getRandomValues symbol on invocation, rather than caching it at module load time. Then at least you just need the polyfill present by the point of use.

Describe alternatives you've considered

See above list. Option 1 seems like the best choice to me, though option 2 would at least restore the pre-7.x functionality. The other two are mainly workarounds for not doing the first one. Relying on module load order is really the core issue, and until uuid stops relying on module load order it will continue to have this problem.

@ctavan
Copy link
Member

ctavan commented Nov 20, 2020

@drewthaler thank you for raising this. However there are several reasons why we do it the way we do it:

  1. uuid could directly rather than indirectly depend on this module that it certainly depends on, importing a symbol and using that, rather than hoping the polyfill gets installed in time.

The biggest use case for this library is still with Node.js and not React Native. Adding a React Native specific polyfill as a dependency in package.json is not an option.

  1. uuid could fold a minimal version of the polyfill code (which is rather simple) into the module and use that as a fallback

It was a deliberate decision to drop the insecure random number generator fallback that was present up until [email protected], feel free to read through #173 and related issues (like #354).

  1. uuid could lazy-load the getRandomValues symbol on invocation, rather than caching it at module load time. Then at least you just need the polyfill present by the point of use.

This is an option I would like to explore a bit more. Right now we only assign the symbol at module parse time because of the fallback for IE11 msCrypto:

uuid/src/rng-browser.js

Lines 5 to 13 in f3bd455

// getRandomValues needs to be invoked in a context where "this" is a Crypto implementation. Also,
// find the complete implementation of crypto (msCrypto) on IE11.
const getRandomValues =
(typeof crypto !== 'undefined' &&
crypto.getRandomValues &&
crypto.getRandomValues.bind(crypto)) ||
(typeof msCrypto !== 'undefined' &&
typeof msCrypto.getRandomValues === 'function' &&
msCrypto.getRandomValues.bind(msCrypto));

I think we can find a different way where we don't "cache" the symbol on module parse time.

All that said uuid is a general purpose JavaScript library. Unfortunately the React Native runtime does not provide a secure pseudo random number generator, but from my point of view it is out-of-scope for this library to take care of polyfilling.

BTW even Node.js has recently added the WebCrypto API and I think at least for Node.js there's a general tendency to converge the core libraries towards the Web libraries (see e.g. WHATWG URL, Fetch and WebCrypto). So my hope would be that at some point in time React Native also jumps on that train and starts providing a reasonable set of web APIs. Until then I believe the best thing we can do is polyfilling the missing behavior.

I do see the issue of module load order though and I'm happy to accept a pull request that would get rid of the getRandomValues() symbol caching.

@drewthaler
Copy link
Contributor Author

Thanks for the quick response! I certainly understand, having written code that lives in multiple contexts myself. I'm curious what percentage of installations (users, vs developers) are using uuid in a react-native project. Is there any way to track that? Since Expo uses uuid, it seems possible that the number of RN clients might be much higher than you think. Issues with this code might start trickling in as projects upgrade their dependencies.

I'm not saying that RN should guide the development of uuid simply because Expo depends on it -- but there's a friction there, as this change introduces real problems for react-native users. Anyway, back to the issue at hand:

Lazy-loading the polyfill would just involve pushing the initialization down into the rng() function, something like this:

let getRandomValues;

export default function rng() {
  if (!getRandomValues) {
    getRandomValues = 
      (typeof crypto !== 'undefined' && 
        crypto.getRandomValues && 
        crypto.getRandomValues.bind(crypto)) || 
      (typeof msCrypto !== 'undefined' && 
        typeof msCrypto.getRandomValues === 'function' && 
        msCrypto.getRandomValues.bind(msCrypto));
    if (!getRandomValues) {
      throw new Error(
        'crypto.getRandomValues() not supported. See https://github.com/uuidjs/uuid#getrandomvalues-not-supported'
      );
    }
  }

  return getRandomValues(rnds8);
}

This shouldn't even noticeably slow anything down, as it doesn't add a new branch in the steady-state case. (The if check for initialization runs every time already.)

@ctavan
Copy link
Member

ctavan commented Nov 21, 2020

Thanks for the quick response! I certainly understand, having written code that lives in multiple contexts myself. I'm curious what percentage of installations (users, vs developers) are using uuid in a react-native project. Is there any way to track that?

Out of curiosity I went back to the analysis I once made for the uuid TC39 proposal where I analyzed the public BigQuery Github dataset.

What I basically did was getting all package.json files from this dataset that have uuid as a dependency and then checking, whether they contain react-native as a dependency as well. The result was:

Has React Native dep? Count Ratio
false 6353 0.98
true 122 0.02

So out of all projects in the BigQuery GitHub dataset that declare uuid as a dependency only about 2% seem to be react-native projects.

@drewthaler
Copy link
Contributor Author

Confirmed the new release works correctly, thanks! It's actually being used by an upstream dependency of ours so I'll push that dependency to adopt it as well.

Regarding the analysis of react-native use, pardon the rabbit hole here, but I was curious and dug into it a little bit. It appears the analysis code is only looking at first-order dependencies, and possibly only some of those? I'm not sure.

I was curious and figured I'd just check how many projects used Expo. Expo is Facebook's preferred fast-and-easy way to use RN, and Expo definitely depends on uuid (by way of expo-constants, expo-location, expo-file-system, expo-notifications, expo-updates, and others),

I took the analysis code and got it running, confirmed that it gave some results for "uuid" dependencies, and then modified it (basically a replace uuid->expo) to look for "expo" dependencies, and dumped the result into a new dataset.

The result was... zero hits.

That's weird because I would have expected, for example, https://github.com/ethantran/react-native-examples to show up in the query as that definitely has a direct dependency on expo. I'm a little stumped here as to what might be going wrong.

But anyway, it's hard to say without hard data, but there are certainly more than 122 expo-based projects on github. :) I think perhaps if you are able to look beyond direct dependencies into indirect dependencies you'll find that there are more RN clients than you might think.

@ctavan
Copy link
Member

ctavan commented Dec 8, 2020

@drewthaler that's interesting. Would probably require some more investigation. However, whatever the outcome, it would still not convince me to include the RN polyfill as a dependency so probably not worth going down that rabbit hole right now :).

I'll go ahead and publish a non-beta version now so that all RN users can benefit from your fix.

Vylpes pushed a commit to Vylpes/Droplet that referenced this issue Sep 14, 2023
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [uuid](https://github.com/uuidjs/uuid) | dependencies | major | [`^8.3.2` -> `^9.0.0`](https://renovatebot.com/diffs/npm/uuid/8.3.2/9.0.0) |
| [@types/uuid](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/uuid) ([source](https://github.com/DefinitelyTyped/DefinitelyTyped)) | dependencies | major | [`^8.3.0` -> `^9.0.0`](https://renovatebot.com/diffs/npm/@types%2fuuid/8.3.0/9.0.1) |

---

### Release Notes

<details>
<summary>uuidjs/uuid</summary>

### [`v9.0.0`](https://github.com/uuidjs/uuid/blob/HEAD/CHANGELOG.md#&#8203;900-httpsgithubcomuuidjsuuidcomparev832v900-2022-09-05)

[Compare Source](uuidjs/uuid@v8.3.2...v9.0.0)

##### ⚠ BREAKING CHANGES

-   Drop Node.js 10.x support. This library always aims at supporting one EOLed LTS release which by this time now is 12.x which has reached EOL 30 Apr 2022.

-   Remove the minified UMD build from the package.

    Minified code is hard to audit and since this is a widely used library it seems more appropriate nowadays to optimize for auditability than to ship a legacy module format that, at best, serves educational purposes nowadays.

    For production browser use cases, users should be using a bundler. For educational purposes, today's online sandboxes like replit.com offer convenient ways to load npm modules, so the use case for UMD through repos like UNPKG or jsDelivr has largely vanished.

-   Drop IE 11 and Safari 10 support. Drop support for browsers that don't correctly implement const/let and default arguments, and no longer transpile the browser build to ES2015.

    This also removes the fallback on msCrypto instead of the crypto API.

    Browser tests are run in the first supported version of each supported browser and in the latest (as of this commit) version available on Browserstack.

##### Features

-   optimize uuid.v1 by 1.3x uuid.v4 by 4.3x (430%) ([#&#8203;597](uuidjs/uuid#597)) ([3a033f6](uuidjs/uuid@3a033f6))
-   remove UMD build ([#&#8203;645](uuidjs/uuid#645)) ([e948a0f](uuidjs/uuid@e948a0f)), closes [#&#8203;620](uuidjs/uuid#620)
-   use native crypto.randomUUID when available ([#&#8203;600](uuidjs/uuid#600)) ([c9e076c](uuidjs/uuid@c9e076c))

##### Bug Fixes

-   add Jest/jsdom compatibility ([#&#8203;642](uuidjs/uuid#642)) ([16f9c46](uuidjs/uuid@16f9c46))
-   change default export to named function ([#&#8203;545](uuidjs/uuid#545)) ([c57bc5a](uuidjs/uuid@c57bc5a))
-   handle error when parameter is not set in v3 and v5 ([#&#8203;622](uuidjs/uuid#622)) ([fcd7388](uuidjs/uuid@fcd7388))
-   run npm audit fix ([#&#8203;644](uuidjs/uuid#644)) ([04686f5](uuidjs/uuid@04686f5))
-   upgrading from uuid3 broken link ([#&#8203;568](uuidjs/uuid#568)) ([1c849da](uuidjs/uuid@1c849da))

##### build

-   drop Node.js 8.x from babel transpile target ([#&#8203;603](uuidjs/uuid#603)) ([aa11485](uuidjs/uuid@aa11485))

-   drop support for legacy browsers (IE11, Safari 10) ([#&#8203;604](uuidjs/uuid#604)) ([0f433e5](uuidjs/uuid@0f433e5))

-   drop node 10.x to upgrade dev dependencies ([#&#8203;653](uuidjs/uuid#653)) ([28a5712](uuidjs/uuid@28a5712)), closes [#&#8203;643](uuidjs/uuid#643)

##### [8.3.2](uuidjs/uuid@v8.3.1...v8.3.2) (2020-12-08)

##### Bug Fixes

-   lazy load getRandomValues ([#&#8203;537](uuidjs/uuid#537)) ([16c8f6d](uuidjs/uuid@16c8f6d)), closes [#&#8203;536](uuidjs/uuid#536)

##### [8.3.1](uuidjs/uuid@v8.3.0...v8.3.1) (2020-10-04)

##### Bug Fixes

-   support expo>=39.0.0 ([#&#8203;515](uuidjs/uuid#515)) ([c65a0f3](uuidjs/uuid@c65a0f3)), closes [#&#8203;375](uuidjs/uuid#375)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about these updates again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC43NC4yIiwidXBkYXRlZEluVmVyIjoiMzQuNzQuMiJ9-->

Co-authored-by: Renovate Bot <[email protected]>
Reviewed-on: https://gitea.vylpes.xyz/RabbitLabs/Droplet/pulls/111
Reviewed-by: Vylpes <[email protected]>
Co-authored-by: RenovateBot <[email protected]>
Co-committed-by: RenovateBot <[email protected]>
@broofa
Copy link
Member

broofa commented Oct 28, 2024

This is an option I would like to explore a bit more. Right now we only assign the symbol at module parse time because of the fallback for IE11 msCrypto:

FWIW, we no longer have the msCrypto check. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment