-
-
Notifications
You must be signed in to change notification settings - Fork 45
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
Require Node.js 14.16 #44
Conversation
Signed-off-by: Richie Bendall <[email protected]>
Signed-off-by: Richie Bendall <[email protected]>
Signed-off-by: Richie Bendall <[email protected]>
Signed-off-by: Richie Bendall <[email protected]>
e92d4e1
to
31efb79
Compare
Signed-off-by: Richie Bendall <[email protected]>
Signed-off-by: Richie Bendall <[email protected]>
743dd34
to
2db06d0
Compare
Signed-off-by: Richie Bendall <[email protected]>
2db06d0
to
819c9ba
Compare
Signed-off-by: Richie Bendall <[email protected]>
Did you test it in the browser? Does the |
Co-authored-by: Sindre Sorhus <[email protected]>
I switched to To test browser support, the only browser API that is used, I've manually verified that the browser code runs in Chrome 100. |
Signed-off-by: Richie Bendall <[email protected]>
…random-string into require-node-14
You can still do that with AVA. I know it's not as nice, but I would strongly prefer to use AVA. Not because I made it, but because it's what I use in all my packages. Probably one day the You can just call AVA twice with different argument and check for that in the test file: https://github.com/avajs/ava/blob/main/docs/recipes/passing-arguments-to-your-test-files.md#passing-arguments-to-your-test-files AVA may some day get nicer ways to do this kind of thing: avajs/ava#2980 And new issues are always welcome if you can think of a nice way for AVA to handle this. Maybe AVA could have some kind of matrix functionality, like GitHub Actions. |
Thought I had seen it used in another pull request that was merged but turns out that it wasn't in one of your repos. Will fix. |
Signed-off-by: Richie Bendall <[email protected]>
@@ -48,7 +48,7 @@ cryptoRandomString({length: 10, characters: 'abc'}); | |||
|
|||
Returns a randomized string. [Hex](https://en.wikipedia.org/wiki/Hexadecimal) by default. | |||
|
|||
### cryptoRandomString.async(options) | |||
### cryptoRandomStringAsync(options) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to indicate somehow that this is a named export and the other one is a default export.
@Richienb bump |
This is quite a challenging task, haven't found inspiration from other repositories yet. My initial idea was to make the first function name italic but it didn't appear visible enough. |
For now, I think the best way is just a code a code example that includes the import. |
Extra sanity check added in Node.js 16.15.0 affected the unit tests, fixed now |
Nice work |
The next Node.js version that will actually allow us to improve the code is v16 which has the web crypto API built-in, and v18 which has it exposed as a global.
Still need to test browser support.