Page MenuHomePhabricator

[web] Add a function for generating random strings
ClosedPublic

Authored by inka on Aug 3 2022, 6:57 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Nov 18, 3:15 PM
Unknown Object (File)
Mon, Nov 18, 3:14 PM
Unknown Object (File)
Mon, Nov 18, 3:14 PM
Unknown Object (File)
Mon, Nov 18, 3:14 PM
Unknown Object (File)
Mon, Nov 18, 3:14 PM
Unknown Object (File)
Mon, Nov 18, 3:14 PM
Unknown Object (File)
Mon, Nov 18, 3:14 PM
Unknown Object (File)
Mon, Nov 18, 3:14 PM

Details

Summary

Add a function for generating random strings on web. Add tests for the function.
This function will be used for generating device ID on web, issue: https://linear.app/comm/issue/ENG-1279/initialize-deviceid-on-clients

Test Plan

Run provided tests.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

inka requested review of this revision.Aug 3 2022, 7:09 AM
tomek requested changes to this revision.Aug 3 2022, 8:38 AM

It might help other reviewers to know that this solution was inspired by the implementation https://github.com/sindresorhus/crypto-random-string/blob/main/core.js from crypto-random-string library.

web/utils/text-utils.js
24 ↗(On Diff #15263)

If we want to have thorough preconditions checking, we need to also verify that the length of availableSigns isn't greater than numberOfPossibleByteValues.

33–34 ↗(On Diff #15263)

It might be a good idea to add a comment explaining why we're using this value. Also, shouldn't we take a floor of this?

This revision now requires changes to proceed.Aug 3 2022, 8:38 AM
web/utils/text-utils.test.js
15 ↗(On Diff #15263)

You can use template literals https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals to make this more readable

Add an invariant, a comment, minor fix to calculations and make regexes
more readable

tomek requested changes to this revision.Aug 4 2022, 2:15 AM
tomek added inline comments.
web/utils/text-utils.js
31 ↗(On Diff #15300)

We should avoid using duplicated values if there's a connection between them. Here we're comparing with numberOfPossibleByteValues so this value should be used instead of the hardcoded one.

web/utils/text-utils.test.js
38 ↗(On Diff #15300)

Please take care of the line length.

44–47 ↗(On Diff #15300)

To make it more readable we can use repeat function of string

This revision now requires changes to proceed.Aug 4 2022, 2:15 AM
tomek requested changes to this revision.Aug 4 2022, 5:57 AM
tomek added inline comments.
web/utils/text-utils.js
1–2 ↗(On Diff #15303)

Please add an empty line after @flow

2 ↗(On Diff #15303)

It looks like importing crypto, which is a node module, works because it gets polyfilled. The curious thing is that no action was taken on our side to make this working. This happened because in webpack 4 the default configuration https://v4.webpack.js.org/configuration/node/ is global: true which means that node globals were polyfilled by default. This could also means that we were already including this code in our builds (unless some tree shakings were performed).
In webpack 5 https://webpack.js.org/configuration/node/ this defaults change to global: false, so during the upgrade we would need to either set it to true or find some alternative solution.

41–44 ↗(On Diff #15303)

I guess my comment was confusing. Taking floor of validByteUpperBound / numberOfPossibleByteValues doesn't make sense as it is almost always less than 1. We should instead take a floor of the whole expression.

This revision now requires changes to proceed.Aug 4 2022, 5:57 AM
web/utils/text-utils.js
41–44 ↗(On Diff #15303)

Right! This makes no sense at all, I'm sorry

This revision is now accepted and ready to land.Aug 4 2022, 6:34 AM