search

holochain / holochain-client-js

A JavaScript client for the Holochain Conductor API
Other
258 stars 15 forks source link

Complete integration of `@hc-spartan/holo-hash` or revert #265

Closed mattyg closed 2 months ago

mattyg commented 3 months ago

This PR #259 introduced some bugs: by using the classes in @spartan-hc/holo-hash for type definitions it makes it appear that the types you are getting back from client calls are instances of those classes, but they're not.

We either need to modify client responses to transform Uint8Arrays into their expected @spartan-hc/holo-hash class instances, or to revert.

matthme commented 3 months ago

We either need to modify client responses to transform Uint8Arrays into their expected @spartan-hc/holo-hash class instances

I would advocate against using objects in client responses as that will create interoperability issues if someone is using a different js-client. I already noticed that the latest PR breaks apps using that new client within Moss which is using an older version of the client because Moss expects pure Uint8Arrays in iframe messages from apps but it gets javascript objects instead.

matthme commented 3 months ago

I think it would be smart to stay with the pure serialized responses as anyone would get them when decoding websocket responses from javascript and keep these classes as optional sugar to make things more ergonomic in other contexts.

guillemcordoba commented 3 months ago

I agree... Right now I think the PR makes the types for the responses coming from the AdminWebsocket just incorrect, as typescript would need the HoloHash classes to compile, but then when the responses come they are just Uint8Arrays in reality. That could be addressed in the transformer for the AdminWebsocket to convert Uint8Arrays into HoloHashes. However, the core @holochain/client package can't do things like go over all response types that the app call zomes return, and a bunch of them will be Uint8Arrays. So if this change stays, the app developers would be left with a weird mixture of HoloHashes and Uint8Array, and it may not be clear to them when they are receiving what, and also it's more confusing because at the conceptual level they are equivalent.

I think for this to be properly implemented it should be at the level of msgpack's extension types, where one of the extension types would be HoloHash. I'm not sure exactly how this would need to be implemented, but I think it also needs work from the holochain core side to mark HoloHashes as special rmp-serde types. If done this way, I would support it wholeheartedly, and could even contribute to it. The biggest win for me here is that we could declare the equals function of HoloHashes as equality by value and not by reference, and then hash1 === hash2 would just work as expected (right now it doesn't, we need to do weird things like hash1.toString() === hash2.toString().

jost-s commented 2 months ago

Reverting for now. I think the JS client could provide its own class for hashes as is implemented by @hc-spartan/holo-hash with useful methods and proper comparison operators, but would need to convert all Uint8Array response types that are hashes to instances of this class, as you all pointed out.

I don't have a strong preference here. If the majority prefer to have hashes be a specific class, I'm happy to properly implement that with more thought this time.

Regarding the implementation, msgpack extension types could be used to encode that the value is of a hash type or even which type of hash it is. In my opinion that's an additional layer of type consistency and the JS client implementation of hash types does not depend on it.