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
Details
Run provided tests.
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
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? |
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
web/utils/text-utils.js | ||
---|---|---|
31 | 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 | Please take care of the line length. | |
44–47 | To make it more readable we can use repeat function of string |
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). |
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. |
web/utils/text-utils.js | ||
---|---|---|
41–44 ↗ | (On Diff #15303) | Right! This makes no sense at all, I'm sorry |