Page MenuHomePhabricator

[web] add std::optional (C++) <-> undefined (JS) marshalling
ClosedPublic

Authored by ashoat on Jul 14 2024, 6:06 PM.
Tags
None
Referenced Files
F3181547: D12736.diff
Fri, Nov 8, 6:26 AM
Unknown Object (File)
Fri, Oct 18, 10:29 PM
Unknown Object (File)
Fri, Oct 18, 10:29 PM
Unknown Object (File)
Fri, Oct 18, 10:29 PM
Unknown Object (File)
Fri, Oct 18, 10:24 PM
Unknown Object (File)
Sep 19 2024, 9:30 AM
Unknown Object (File)
Sep 16 2024, 2:58 AM
Unknown Object (File)
Sep 16 2024, 2:58 AM
Subscribers
None

Details

Summary

This follows a similar approach outlined in D8552. I was inspired by searching the string "BindingType<std::optional" on GitHub. This project provided a good implementation.

Depends on D12735

Test Plan

I tested this as part of the getLatestMessageEdit API I introduce later in this stack. I tested that on both web and iOS, which required using the std::optional <-> undefined marshalling

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

It's great to see these bindings! At some point, we should delete NullableString and NullableInt.

The diff looks reasonable but @kamil should also take a look.

This revision is now accepted and ready to land.Jul 15 2024, 1:46 AM

Making @kamil blocking based on @tomek's comment above.

It's great to see these bindings! At some point, we should delete NullableString and NullableInt.

We should also be able to get rid of the web-specific getOlmPersistAccountDataWeb and just use getOlmPersistAccountData on web. I'll create a follow-up task for this before landing.

This revision now requires review to proceed.Jul 15 2024, 8:07 AM

I ended up not needing this, since I'm replacing the getLatestMessageEdit API with a getRelatedMessages API that returns an array.

That said, I think it's still work landing. Going to separate it out from the rest of my stack though.

This revision is now accepted and ready to land.Jul 16 2024, 2:55 AM

We should also be able to get rid of the web-specific getOlmPersistAccountDataWeb and just use getOlmPersistAccountData on web. I'll create a follow-up task for this before landing.

Task with some context: ENG-5719.

Ah okay, no need to create a new follow-up task then – thanks for linking!