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

Issue #474 #475

Closed
wants to merge 6 commits into from
Closed

Issue #474 #475

wants to merge 6 commits into from

Conversation

Paganiniana
Copy link

Salutations and hello, all!

This PR is related to issue #474, reported, yesterday. This is my current workaround that I'm using to continue to make use of the project. It all comes down to using a simple serialized format for Google's Timestamp type, re-initializing it whenever it passes between the web and the native layers, but it probably shouldn't be merged as is. This is my first contribution to CapAwesome and I'm still getting the lay of the land (what's more, my Swift is pretty weak). To constitute a full solution, I'm imagining something as follows....

  1. Recursively check child JSObjects for Timestamp types and convert as needed (this commit is only a superficial check).

I'm imagining that this would involve the following:

  1. Creating a local extension of JSType that includes Timestamp as a potential value
  2. Moving the serialization logic somewhere else

@robingenz, I should have more time this weekend to work on my Swift-ese and create a more elegant solution on the iOS side. I could probably figure out the Android side, too, but I don't particularly want to. 😅

@robingenz
Copy link
Member

Thank you! I really like your proposed solution. I will take over from here and work on an Android implementation as well. Probably this solution will help me with #443 as well, but it will take me a few weeks as I have other priorities at the moment. I'll keep you updated!

@robingenz robingenz added this to the v5.3.0 milestone Oct 24, 2023
@Paganiniana
Copy link
Author

Thanks for getting on this so fast, @robingenz — you're a gem. 👏 I'll have time to help flesh out the iOS side if you like. I actually just made a push to fix the "check recursive" bug, so it should convert timestamps at all layers, now.

@robingenz
Copy link
Member

BTW: The Firebase JavaScript SDK should remain an optional dependency on Android and iOS. I will therefore replace the imported Timestamp of the Firebase JavaScript SDK in the final implementation.

@Paganiniana
Copy link
Author

BTW: The Firebase JavaScript SDK should remain an optional dependency on Android and iOS. I will therefore replace the imported Timestamp of the Firebase JavaScript SDK in the final implementation.

That makes total sense — I wound up having to avoid it here, anyway, because the class name seemed to get clobbered by various build tools. (Passing a Timestamp from my web app did not get recognized as an instanceof Timestamp at the plugin layer). I started fingerprinting it by the nanoseconds property — not super thorough.

@robingenz robingenz modified the milestones: v5.3.0, v5.4.0 Nov 28, 2023
@robingenz robingenz modified the milestones: v5.4.0, v5.5.0 Jan 23, 2024
@robingenz
Copy link
Member

I close this for #557.

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.

2 participants