Add JS target

[luca992]

I added JS as a target.
However I have some questions:

1. I changed NATIVE_TARGETS_ENABLED -> NONJVM_TARGETS_ENABLED to cover both native and JS targets. Or would you want two separate flags, or some other setup?
2. ReadmeExamplesTest are all failing on JS due to IllegalStateException: Nested tests are not supported Support for nested tests on JS target (again) kotest/kotest#3141 Any suggestions on alternatives ways I could rewrite those tests to not use context (which is what I think is the issue)?
3. I wanted to get another opinion of whether or not getRandomValues is sufficiently secure for this libraries purposes? Or should I swap it out for generateKey()

Author

• Self-review your own code in GitHub's web interface¹
• Add automated tests as appropriate
• Check the code coverage² report for the automated tests
• Update documentation as appropriate (e.g README.md, etc.)
• Pull in the latest changes from the main branch and squash your commits before assigning a reviewer³

Reviewer

• Check the code with the Code Review Guidelines checklist
• Perform an ad hoc review⁴
• Review the automated tests
• Review the documentation, README.md, etc. as appropriate

Footnotes

1. Code often looks different when reviewing the diff in a browser, making it easier to spot potential bugs. ↩

2. While we are not looking for perfect coverage, the tool can point out potential cases that have been missed. Code coverage can be generated with: ./gradlew check. ↩

3. Having your code up to date and squashed will make it easier for others to review. Use best judgement when squashing commits, as some changes (such as refactoring) might be easier to review as a separate commit. ↩

4. In addition to a first pass using the code review guidelines, do a second pass using your best judgement and experience which may identify additional questions or comments. Research shows that code review is most effective when done in multiple passes, where reviewers look for different things through each pass. ↩

[ccjernigan]

I added JS as a target. However I have some questions:

1. I changed NATIVE_TARGETS_ENABLED -> NONJVM_TARGETS_ENABLED to cover both native and JS targets. Or would you want two separate flags, or some other setup?

If you don't mind, let's separate the native and JS targets. That will let us incrementally set up the multiplatform deployment with a subset of targets. We'll leave the flags enabled by default in the gradle.properties but then override it in the GitHub Actions deployment script to avoid deploying the non-JVM targets initially. When we're ready to deploy, JS will probably be turned on first since it won't require setting up macOS runners.

I used the NATIVE_TARGETS_ENABLED flag on my machine earlier this week, because Xcode was having a meltdown about the tvOS and watchOS targets. Having those flags available has already been quite useful for local development too.

2. ReadmeExamplesTest are all failing on JS due to IllegalStateException: Nested tests are not supported Support for nested tests on JS target (again) kotest/kotest#3141 Any suggestions on alternatives ways I could rewrite those tests to not use context (which is what I think is the issue)?

Yeah, that's tricky.

Long term, I hope to eliminate Kotest because I don't think the added dependency brings us large benefits over simply leveraging Kotlin's built-in test assertions (e.g. assertTrue, assertEquals in some simple @Test annotated methods). As a company, we aren't using kotest in our other projects.

Would it make sense to just switch to Kotlin test methods to bypass the context entirely? From my perspective, it is OK to just modify the ReadmeExamplesTest for now.

I'm open to other ideas, although the suggestion above moves us in the direction I think the project should go longer term.

3. I wanted to get another opinion of whether or not getRandomValues is sufficiently secure for this libraries purposes? Or should I swap it out for generateKey()

Since I'm less familiar with the JS random APIs, let's get input from @hashben228.

[ccjernigan]

One additional thing I know that we'll need to also update: Modify the dependency update issue template in the .github folder to include the kotlinUpgradeYarnLock task. This is mostly to help people new to multiplatform remember which tasks to run

[HonzaR]

Hi @luca992, Thank you for this PR! Are you still interested in finishing it? I believe we're very close to having it done. I'm also not very familiar with the JS platform, but hopefully, I could find someone in the company to assist us here, if needed.

[luca992]

Hi @luca992, Thank you for this PR! Are you still interested in finishing it? I believe we're very close to having it done. I'm also not very familiar with the JS platform, but hopefully, I could find someone in the company to assist us here, if needed.

Yeah I was just waiting for another opinion on the random-ness source I'm using

[HonzaR]

Yeah I was just waiting for another opinion on the random-ness source I'm using

I understand, we have someone around, so I'll manage it and let you know. Thank you for your patience.

[luca992]

Another crypto library is using getRandomValues here:

https://github.com/whyoleg/cryptography-kotlin/blob/main/cryptography-random/src/jsMain/kotlin/CryptographyRandom.js.kt

[HonzaR]

Another crypto library is using getRandomValues here:

Alright, that looks good to me. Although we have no JS-oriented colleague around now, we could proceed to agree on this solution. Could you please rebase and fix the failing test and detekt warning?

[luca992]

@HonzaR @ccjernigan finally got the motivation to finish this.

I completely removed kotest. Turns out none of the tests would run on js, not just ReadmeExamplesTest

I think I addressed everything brought up, and should be ready to review. Also, I ran ktlint --format on the whole project so that's why there's a bunch of formatting changes even outside the src folder.

Ps. I would like to add the wasmJs after this, just probably just work out of the box.