signalapp / libsignal

Home to the Signal Protocol as well as other cryptographic primitives which make Signal possible.
GNU Affero General Public License v3.0
3.08k stars 362 forks source link

Extending Typescript wrapper classes #438

Closed JoshStern closed 2 years ago

JoshStern commented 2 years ago

Hey! I was hoping to extend the PublicKey and other Native wrapper classes. I noticed they all mark their constructor as private.

I was wondering:

  1. What's the motivation for not allowing extension?
  2. Would making the constructors protected be an acceptable compromise?

I can always create a wrapper of my own if that's what you recommend instead.

PS - Reading your code has taught me a ton so far 😄. Looking forward to more tinkering.

jrose-signal commented 2 years ago

Keeping the constructors private allows us to do things like construct wrapper objects from the Rust side of the bridge, although we're not doing so at the moment. If we went down that path, you'd end up with some objects using your custom subclass and some using the default class.

To flip the question around, what are you hoping to do with a subclass?

JoshStern commented 2 years ago

I have a serialization interface I'd like to fulfill for anything getting sent over the network/held in a message queue. PublicKey can be serialized to a buffer with the current API but I'd like to build out a uniform interface for all of them.

For now I have separate functions that handle encoding/decoding which works but isn't ideal.

I could also use serde on the rust side to add more bridge methods but that'd have to be in a fork and I was hoping to avoid maintaining my own.

jrose-signal commented 2 years ago

It should be fine to add those methods to the SignalClient classes directly, no? Either they won't conflict with anything, or they do conflict and it'd be a problem in a subclass too.

JoshStern commented 2 years ago

To clarify, when you say add directly do you mean attaching it to the class prototypes in my own project or to the node/ts/index.ts class declarations in the lib?

I messed around with mutating the class, which works but I wanted to see if there's a more direct route.

Adding to the lib is an option too, my thought was if I fork the project then I might as well let serde do the heavy lifting + generate implementations.

jrose-signal commented 2 years ago

Yeah, I meant mutating the SignalClient class from your project ("module augmentation"). It's not so cleanly supported in TypeScript compared to type-less, prototype-accessible JavaScript, but it should get the job done.

JoshStern commented 2 years ago

Sounds good, I'll go with that option. Typescript shouldn't be too much trouble to correct with a little abstraction. Thank you for taking the time to answer and for the quick responses! I'll close this one out.