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

remove genUUID and unused useRandom hook #585

Closed

Conversation

balegas
Copy link
Contributor

@balegas balegas commented Oct 23, 2023

Use of crypto was creating problems with some platforms. This was also redundant because we also define a uuid() function.

@linear
Copy link

linear bot commented Oct 23, 2023

@balegas balegas force-pushed the vbalegas/vax-1246-remove-unused-random-generator-functions branch from dcceccb to 73c010f Compare October 23, 2023 10:38
@balegas balegas marked this pull request as draft October 23, 2023 10:58
@thruflo
Copy link
Contributor

thruflo commented Oct 23, 2023

genUUID is used in a bunch of places. Examples, docs, etc.

@balegas
Copy link
Contributor Author

balegas commented Oct 23, 2023

yep, pushed another commit right now. Checking if examples are still working (website is okay)

@balegas
Copy link
Contributor Author

balegas commented Oct 23, 2023

Tested with Expo and React native to double-check that uuid() polyfills are working on mobile environments.

Had to fiddle with React Native to get the code working again. I initially thought that this was because of upgrading to Sonoma, but apparently it is a global issue: CocoaPods/CocoaPods#12081

I got things working for me. @kevin-dp @thruflo could one of you check that React Native is also broken for you. If yes, I'll provide instructions.

@balegas balegas requested a review from thruflo October 23, 2023 14:51
@balegas balegas marked this pull request as ready for review October 23, 2023 14:52
@balegas
Copy link
Contributor Author

balegas commented Oct 23, 2023

  • revert unintended formatting changes

@balegas balegas force-pushed the vbalegas/vax-1246-remove-unused-random-generator-functions branch 4 times, most recently from 451851f to 90a5e52 Compare October 23, 2023 15:17
@balegas balegas force-pushed the vbalegas/vax-1246-remove-unused-random-generator-functions branch from 90a5e52 to c11ea92 Compare October 23, 2023 15:20
@thruflo
Copy link
Contributor

thruflo commented Oct 23, 2023

Attempting to verify. If I can get an 8GB iOS Simulator upgrade down over pub wifi ...

@thruflo
Copy link
Contributor

thruflo commented Oct 24, 2023

React Native works fine for me.

@thruflo
Copy link
Contributor

thruflo commented Oct 24, 2023

Screenshot 2023-10-24 at 11 45 03

@balegas
Copy link
Contributor Author

balegas commented Oct 24, 2023

Cool. No hassle installing ruby libraries?

@thruflo
Copy link
Contributor

thruflo commented Oct 24, 2023

Only with my own Xcode tooling.

@balegas
Copy link
Contributor Author

balegas commented Oct 30, 2023

Will be merging this since changes are not related with the instructions for React Native failing for me. Created a new issue to review React Native instructions with more recent version of cocoa pods.

@balegas
Copy link
Contributor Author

balegas commented Oct 30, 2023

@thruflo still need an approval

@kevin-dp
Copy link
Contributor

@balegas how can i run this?

Because how it is configured it will use the published Electric image. If i do that i get errors. Should i run against a local build? But that's a bit tricky as it requires all the changes to expose the necessary ports.

@balegas
Copy link
Contributor Author

balegas commented Nov 7, 2023

@balegas how can i run this?

Because how it is configured it will use the published Electric image. If i do that i get errors. Should i run against a local build? But that's a bit tricky as it requires all the changes to expose the necessary ports.

@kevin-dp not sure if I missed something, but the answer is the same way you tested applications before, because nothing changed.

@kevin-dp
Copy link
Contributor

kevin-dp commented Nov 9, 2023

@balegas how can i run this?
Because how it is configured it will use the published Electric image. If i do that i get errors. Should i run against a local build? But that's a bit tricky as it requires all the changes to expose the necessary ports.

@kevin-dp not sure if I missed something, but the answer is the same way you tested applications before, because nothing changed.

I don't think this works since we introduced the proxy and now the latest published image also uses the proxy but this app is not using the proxy.

@balegas
Copy link
Contributor Author

balegas commented Nov 14, 2023

@samwillis pulling you in on this one after conversation yesterday.
I opened this PR because genUUID() was causing problems and I decided to converge code to using uuid().

As you point out, we're overriding a global variable, so maybe we need to handle this better. We do that to ensure we use the right dep for the different envs. If we have one that works across that would simplify code. I think initially we decided not to go with a js-based approach because of higher collision rate. I'm happy with that tradeoff.

If you wish to fix this as part of your refactoring, I'm very happy. Otherwise, leave a comment here so we can pick it up later.

@balegas
Copy link
Contributor Author

balegas commented Dec 5, 2023

I'm, closing this PR and requesting #581 to be solved in VAX-1352

@balegas balegas closed this Dec 5, 2023
msfstef added a commit that referenced this pull request Mar 4, 2024
- Removed unused and undocumented `useRandom` hook which was originally
already removed in this PR
#585 but was added back
with another refactor
- Removed dependency on the injected electric client in `useLiveQuery`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants