Page MenuHomePhabricator

[CommCoreModule] Add `openSocket` to `CommCoreModuleSchema`
ClosedPublic

Authored by atul on Feb 7 2022, 7:27 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 27, 6:22 AM
Unknown Object (File)
Nov 10 2024, 11:56 PM
Unknown Object (File)
Nov 10 2024, 11:56 PM
Unknown Object (File)
Nov 10 2024, 11:56 PM
Unknown Object (File)
Nov 10 2024, 11:11 PM
Unknown Object (File)
Nov 5 2024, 12:50 AM
Unknown Object (File)
Oct 24 2024, 11:58 PM
Unknown Object (File)
Oct 24 2024, 11:58 PM

Details

Summary

When initially trying to codegen openSocket, we ran into issues setting the return type to GRPCStream and decided to "generate" the code manually for the time being.

This was confusing for other members of the team as a chunk of the "generated" code would disappear when they tried to codegen "the right way."

Typing the openSocket return type as Object in CommCoreModuleSchema gets around this issue even if it isn't typed perfectly. We were returning a jsi::Object anyways, so this continues to work as expected.

[ignore: depends on D3123]

Test Plan

The GRPCStreamHostObject functionality continued to work as expected

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

atul requested review of this revision.Feb 7 2022, 7:32 AM

glad you figured this out!

This revision is now accepted and ready to land.Feb 7 2022, 7:35 AM
This revision now requires review to proceed.Feb 7 2022, 7:35 AM

It's great to see this fixed

Nice hack. Would be great to confirm that the result of openSocket still gets narrowed downstream to CommTransportLayer, but I'm pretty sure this happens

This revision is now accepted and ready to land.Feb 7 2022, 7:21 PM

Would be great to confirm that the result of openSocket still gets narrowed downstream to CommTransportLayer

It does if we annotate it with the GRPCStream type:

const sock: GRPCStream = global.CommCoreModule.openSocket(...);

Is that what you're asking?

This revision was landed with ongoing or failed builds.Feb 8 2022, 5:24 AM
This revision was automatically updated to reflect the committed changes.

Yeah, that's one way to do it. Or you can have it be returned from some function that you've typed. We just need to make sure that wherever we're using this, we're narrowing the type. The safest thing to do would be to wrap the whole export here in a type wrapper, like how TextInput is handled today

(To be clear I don't think we have time for that right now)