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

Copy expo-random module to crypto #20217

Merged
merged 14 commits into from Nov 29, 2022
Merged

Copy expo-random module to crypto #20217

merged 14 commits into from Nov 29, 2022

Conversation

aleqsio
Copy link
Contributor

@aleqsio aleqsio commented Nov 25, 2022

Why

Two modules have no clear separation of concerns and we want to do significant expansion on the crypto functionality to use the new TypedArrays expo-modules API.

It's easier if we sunset expo-random and use expo-crypto instead, as suggested by @tsapeta.

How

Copied functions, added a test screen to expo-crypto.

Test Plan

Copied existing tests, so the coverage should remain unchanged.

Checklist

@expo-bot expo-bot added bot: suggestions ExpoBot has some suggestions bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Nov 25, 2022
@aleqsio aleqsio marked this pull request as ready for review November 25, 2022 16:09
@aleqsio aleqsio requested review from tsapeta and removed request for tsapeta November 25, 2022 16:09
Copy link
Member

@tsapeta tsapeta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thing to consider that we talked about: add warnings to functions in expo-random, so people upgrading to SDK 48 will see it.

packages/expo-crypto/src/Crypto.ts Outdated Show resolved Hide resolved
packages/expo-crypto/src/Crypto.ts Outdated Show resolved Hide resolved
packages/expo-crypto/src/Crypto.ts Outdated Show resolved Hide resolved
packages/expo-random/CHANGELOG.md Outdated Show resolved Hide resolved
@aleqsio aleqsio requested a review from tsapeta November 28, 2022 11:03
@expo-bot
Copy link
Collaborator

Hi there! 👋 I'm a bot whose goal is to ensure your contributions meet our guidelines.

I've found some issues in your pull request that should be addressed (click on them for more details) 👇

⚠️ Suggestion: Missing changelog entries


Your changes should be noted in the changelog. Read Updating Changelogs guide and consider adding an appropriate entry to the following changelogs:


Generated by ExpoBot 🤖 against 929e2a3

@expo-bot expo-bot added bot: suggestions ExpoBot has some suggestions and removed bot: passed checks ExpoBot has nothing to complain about labels Nov 28, 2022
@aleqsio aleqsio merged commit 6a472ce into main Nov 29, 2022
@aleqsio aleqsio deleted the @aleqsio/move-random-to-crypto branch November 29, 2022 15:40
@KiwiKilian
Copy link
Contributor

KiwiKilian commented Feb 13, 2023

Is there also a migration planned for expo-standard-web-crypto? It still uses expo-random.

Quick workaround otherwise is:

import * as Crypto from 'expo-crypto';

const webCrypto = typeof crypto !== 'undefined' ? crypto : Crypto;

if (typeof crypto === 'undefined') {
  Object.defineProperty(window, 'crypto', {
    configurable: true,
    enumerable: true,
    get: () => webCrypto,
  });
}

But actually I just noticed my usecase is solved with Crypto.randomUUID() – so I can drop uuid which needed the polyfill.

@tsapeta
Copy link
Member

tsapeta commented Feb 13, 2023

Yes, I think we'll eventually migrate expo-standard-web-crypto to use expo-crypto, but since this is just a JS package without native code, we can update it later, even after SDK release 🙂

That's perfect if Crypto.randomUUID() solves your issue though!

creature-water-valley added a commit to ws-4020/mobile-app-crib-notes that referenced this pull request Apr 20, 2023
creature-water-valley added a commit to ws-4020/mobile-app-crib-notes that referenced this pull request Apr 25, 2023
creature-water-valley added a commit to ws-4020/mobile-app-crib-notes that referenced this pull request Apr 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: suggestions ExpoBot has some suggestions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants