@hasgraph/protobuf fails to export Protobuf Types

[bugbytesinc]

Description

The latest version of the JS SDK does not export @hashgrahp/protobuf types as in previous versions, this breaks existing code such as:

Steps to reproduce

Have an existing program written against @hashgraph/proto version 2.1.4, upgrade to 2.2.0 and things fail to compile due to missing type definitions

Additional context

Index.d.ts before the upgrade

index.d.ts after the upgrade

Hedera network

testnet

Version

@hashgraph/proto version 2.2.0

Operating system

Windows

[janaakhterov]

This is my bad, but we recently made breaking changes to @hashgraph/proto (honestly didn't realize others might be using the package as well). The point of this change was to remove our needless copying and pasting of protobuf files whenever we need to update them in the package. The change also removed the need to re-export everything at the top level which was tedious and required us to manually rename things like mirror.ConsensusService to MirrorConsensusServices.

This is how we currently import IConsensusTopicResponse https://github.com/hashgraph/hedera-sdk-js/blob/5092b57b5a88126f84f62679e49c92384917d048/src/topic/TopicMessage.js#L12

I would like to note that typically, I would definitely move backwards and undo the breaking change, but since @hashgraph/proto quite often has breaking changes due to hedera-protobufs having breaking changes (they're allowed to since they're not 1.0) I feel like having this breaking change isn't as harmful. Also, with the current system it should make it many times easier and clearer to move between @hashgraph/proto and hedera-protobufs which I see as a huge upside.

[bugbytesinc]

So how are we to change this file so it works? https://github.com/the-creators-galaxy/hcs-governance/blob/e87037075388ba27a05cbf2e048094f547fdd947/server/src/services/hcs-ballot-processing.service.ts#L1

[janaakhterov]

I'm not entirely sure on how to import namespaces in TS as opposed to JS Doc comments, but if it's anything like JS Doc comments you should update that line with import type { com } from "@hashgraph/proto"; and then replace your use of IConsensusTopicResponse with com.hedera.mirror.api.proto.IConsensusTopicResponse

[bluemonad]

Hello, I was also affected by this change. I use SchedulableTransactionBody in order to decode the base64 code from the mirror API schedule endpoint. I tried fixing it by import { proto } from @hashgraph/proto and then using proto.SchedulableTransactionBody, but it is still undefined. So or now I am forced to use an older version of the SDK.

Besides - even if it did work, that would mean I would now have to import the entire proto namespace

I think it is very problematic to keep all the low level protobuf classes out of reach. Especially when the mirror API forces to use them to do certain things (such as getting details of a scheduled transaction). It would be much appreciated if you could think of a way to keep them available.

It causes another issue for me - I require an ITokenID because this is what SchedulableTransactionBody uses. But now I can no longer export that type. So I am forced to use any and to go "blind", hoping it stays the same. This update makes the Hashgraph SDK a lot less usable and user-friendly to me. Any chance this copying and pasting issue could be solved without hiding so much?

On a side note, from what I've seen, it is generally not recommended to use a lot of namespaces in TypeScript ES6 module context. See the official Typescript documentation: https://www.typescriptlang.org/docs/handbook/namespaces-and-modules.html#needless-namespacing

[janaakhterov]

@bluemonad Can you try using import * as HashgraphProto from "@hashgraph/proto"; and then using HashgraphProto.proto.SchedulableTransactionBody. I know this is much uglier than what we had originally :| but this is what we do in the SDK now. I also know that this doesn't look any different from what you have, but when I was updating the SDK to use the new protobufs exports, I got it work at runtime using * as HashgraphProto. Using import { proto } or * as HashgraphProto both passed TSC, but failed when actually running tests.

Besides - even if it did work, that would mean I would now have to import the entire proto namespace

I'm not completely following with what the issue is here since I think you'd be importing the entire namespace in earlier proto version as well, but silently since the namespace would be imported in @hashgraph/proto instead of your package. Maybe that's my misunderstanding though.

Have you tried exporting the type as export { ITokenID: HashgraphProto.proto.ITokenID };

This update makes the Hashgraph SDK a lot less usable and user-friendly to me. Any chance this copying and pasting issue could be solved without hiding so much?

Trust me, I understand the pain as I had to update the SDK to use the new package. However, even with the pain I feel like this is the only way forward since there were some major issues with the way we had it before. 1) Copying the protobuf files over and including them directly meant there is no obvious way to determine which version of the protobufs we were using. There are ways around this, we could of used a separate file to document the version of the protobufs and whatnot, but ultimately we'd be re-implementing what git sub-modules already does. 2) We had to manually update index.js and index.d.ts to re-export all the types. This made updating the protobufs a hassle and also didn't make sense to me why the SDK should re-export all the types into a single namespace when the protobufs weren't designed that way to begin with. We also needed to rename certain types since both the mirror protobufs and services protobufs exported types with the same name. The renaming also would likely confuse any new users who are not familiar with the protobufs; why is it named ConsensusServices in the hedera-protobufs repo, but in @hashgraph/proto it's named MirrorConsensusServices.

With all this being said, I think the biggest issue is we didn't update the major version of the protobufs when we made this change. I might be worth while to look up how to remove the latest protobuf releases and instead replace a 3.0.0 release instead.

[bluemonad]

Thank you for your response. Maybe you could try adding export next to the namespace/class definitions? Then it should be possible to at least use namespace aliases.

I still think it might be worth it to consider using only modules and then one could just import/export all classes in one file and users will not need to deal with namespaces.

Regardless, in versions 2.12.0 and upwards I am getting a lot of errors like this error when trying to bundle with WebPack (I get no errors in 2.11.3).

ERROR in node_modules\@hashgraph\sdk\lib\account\TransferTransaction.d.ts
207:23-43
[tsl] ERROR in node_modules\@hashgraph\sdk\lib\account\TransferTransaction.d.ts(207,24)
      TS1005: ';' expected.

[bluemonad]

@danielakhterov

2. We had to manually update index.js and index.d.ts to re-export all the types. This made updating the protobufs a hassle and also didn't make sense to me why the SDK should re-export all the types into a single namespace when the protobufs weren't designed that way to begin with. We also needed to rename certain types since both the mirror protobufs and services protobufs exported types with the same name. The renaming also would likely confuse any new users who are not familiar with the protobufs; why is it named ConsensusServices in the hedera-protobufs repo, but in @hashgraph/proto it's named MirrorConsensusServices.

If the reason for it is to avoid naming conflict and re-exporting types, maybe you could consider separating the proto classes to different modules, instead of multiple namespaces in a single file? This would allow anyone to just import the specific module and continue as before (with a little name changing).

As a positive side effect, it should also greatly improve the size of production code that imports just a small part (such as my case), as it should allow a lot more tree shaking, as opposed to one giant namespace.

The nested namespaces structure (especially without export along the namespace hierarchy) is very problematic for users of the SDK and is also not idiomatic to TypeScript.

[janaakhterov]

As a positive side effect, it should also greatly improve the size of production code that imports just a small part (such as my case), as it should allow a lot more tree shaking, as opposed to one giant namespace.

Regardless, in versions 2.12.0 and upwards I am getting a lot of errors like this error when trying to bundle with WebPack (I get no errors in 2.11.3).

@bluemonad The PR linked above attempts to add @hashgraph/sdk to a webpack project. I'm not experience with webpack, so I may be doing something wrong, but from my testing so far it seems @hashgraph/proto v2.11.3 is larger than v2.12.1 which goes against what you're saying. I'm not trying to call you out, but I do want to be able to test my changes. If you see I'm doing something stupid in that PR please feel free to let me know.

The nested namespaces structure (especially without export along the namespace hierarchy) is very problematic for users of the SDK and is also not idiomatic to TypeScript.

I honestly don't see what you mean here. Yes, it is a bit annoying to have nested namespaces because you have to do com.hedera.mirror.api.proto, but I wouldn't say it is very problematic for users of the SDK. I'm not even sure if I'd say it's not idiomatic to be honest.

I still think it might be worth it to consider using only modules and then one could just import/export all classes in one file and users will not need to deal with namespaces.

This is the way I see it: @hashgraph/sdk is meant to be the user focused and hence should offer an easy to use API. @hashgraph/proto is meant to be developer focused, and I'd argue developers who want/need to use the protobufs directly should be aware of the protobuf namespaces since they should be as close to hedera-protobufs as they can be.

[bluemonad]

As a positive side effect, it should also greatly improve the size of production code that imports just a small part (such as my case), as it should allow a lot more tree shaking, as opposed to one giant namespace.

Regardless, in versions 2.12.0 and upwards I am getting a lot of errors like this error when trying to bundle with WebPack (I get no errors in 2.11.3).

@bluemonad The PR linked above attempts to add @hashgraph/sdk to a webpack project. I'm not experience with webpack, so I may be doing something wrong, but from my testing so far it seems @hashgraph/proto v2.11.3 is larger than v2.12.1 which goes against what you're saying. I'm not trying to call you out, but I do want to be able to test my changes. If you see I'm doing something stupid in that PR please feel free to let me know.

I think this is a misunderstanding. I was suggesting that it might be worthwhile to try and refactor so that the classes were defined in separate smaller modules instead of one very large module. That could perhaps allow for more tree shaking when bundling the code. Version 2.11.3 does not do that (classes are defined in the proto namespace).

The nested namespaces structure (especially without export along the namespace hierarchy) is very problematic for users of the SDK and is also not idiomatic to TypeScript.

I honestly don't see what you mean here. Yes, it is a bit annoying to have nested namespaces because you have to do com.hedera.mirror.api.proto, but I wouldn't say it is very problematic for users of the SDK. I'm not even sure if I'd say it's not idiomatic to be honest.

In the current state of the SDK, it is not possible to import protobuf classes. They are hidden within a namespaces without an export to allow access to them. I would argue that's problematic to applications that require them (classes that do not have export cannot be imported).

The official TypeScript documentation strongly suggests to use modules instead of namespaces and recommends not to combine the two (in multiple places). Also see here among many others.

To reiterate why you shouldn’t try to namespace your module contents, the general idea of namespacing is to provide logical grouping of constructs and to prevent name collisions. Because the module file itself is already a logical grouping, and its top-level name is defined by the code that imports it, it’s unnecessary to use an additional module layer for exported objects.

--The TypeScript Handbook

I still think it might be worth it to consider using only modules and then one could just import/export all classes in one file and users will not need to deal with namespaces.

This is the way I see it: @hashgraph/sdk is meant to be the user focused and hence should offer an easy to use API. @hashgraph/proto is meant to be developer focused, and I'd argue developers who want/need to use the protobufs directly should be aware of the protobuf namespaces since they should be as close to hedera-protobufs as they can be.

I'm not sure I follow. In hedera-protobufs all the proto definitions seem to be in separate files in one non-nested folder. If there was, for example, a .ts module for each .proto file there, it would solve the issue to my understanding. People could simply import only the modules that they require (and it would probably also allow for much smaller bundles with tree shaking etc.)

[janaakhterov]

As a positive side effect, it should also greatly improve the size of production code that imports just a small part (such as my case), as it should allow a lot more tree shaking, as opposed to one giant namespace.

Regardless, in versions 2.12.0 and upwards I am getting a lot of errors like this error when trying to bundle with WebPack (I get no errors in 2.11.3).

@bluemonad The PR linked above attempts to add @hashgraph/sdk to a webpack project. I'm not experience with webpack, so I may be doing something wrong, but from my testing so far it seems @hashgraph/proto v2.11.3 is larger than v2.12.1 which goes against what you're saying. I'm not trying to call you out, but I do want to be able to test my changes. If you see I'm doing something stupid in that PR please feel free to let me know.

I think this is a misunderstanding. I was suggesting that it might be worthwhile to try and refactor so that the classes were defined in separate smaller modules instead of one very large module. That could perhaps allow for more tree shaking when bundling the code. Version 2.11.3 does not do that (classes are defined in the proto namespace).

This would then be a feature request, not a bug. Maybe it should go into it's own issue?

The nested namespaces structure (especially without export along the namespace hierarchy) is very problematic for users of the SDK and is also not idiomatic to TypeScript.

I honestly don't see what you mean here. Yes, it is a bit annoying to have nested namespaces because you have to do com.hedera.mirror.api.proto, but I wouldn't say it is very problematic for users of the SDK. I'm not even sure if I'd say it's not idiomatic to be honest.

In the current state of the SDK, it is not possible to import protobuf classes. They are hidden within a namespaces without an export to allow access to them. I would argue that's problematic to applications that require them (classes that do not have export cannot be imported).

The official TypeScript documentation strongly suggests to use modules instead of namespaces and recommends not to combine the two (in multiple places). Also see here among many others.

To reiterate why you shouldn’t try to namespace your module contents, the general idea of namespacing is to provide logical grouping of constructs and to prevent name collisions. Because the module file itself is already a logical grouping, and its top-level name is defined by the code that imports it, it’s unnecessary to use an additional module layer for exported objects.

--The TypeScript Handbook

I still think it might be worth it to consider using only modules and then one could just import/export all classes in one file and users will not need to deal with namespaces.

This is the way I see it: @hashgraph/sdk is meant to be the user focused and hence should offer an easy to use API. @hashgraph/proto is meant to be developer focused, and I'd argue developers who want/need to use the protobufs directly should be aware of the protobuf namespaces since they should be as close to hedera-protobufs as they can be.

I'm not sure I follow. In hedera-protobufs all the proto definitions seem to be in separate files in one non-nested folder. If there was, for example, a .ts module for each .proto file there, it would solve the issue to my understanding. People could simply import only the modules that they require (and it would probably also allow for much smaller bundles with tree shaking etc.)

I don't disagree that the current use of namespaces goes against TypeScripts recommendatation, but I just don't see how that solves any of your issues. If we generated modules instead of namespaces, we'd still have to only export proto from the top level module since exporting all the types within proto could cause typename collisions with for instance com.hedera.mirror.api.proto. Meaning you still wouldn't be able to

import { AccountID } from "@hashgraph/proto";

Also, it's worth pointing out that protobufjs doesn't support outputting modules instead of namespaces. The only reason we have namespaces is because that is what protobufjs generates. I'm not 100% sure, but it may not be possible to generate modules instead of namespaces even if we were to run pbjs per file since pbjs will likely generate new JS types for everything used within a protobuf file even if we have already generated those JS types on a previous run. Another thing we could try is having a separate npm package per protobuf package, so @hashgraph/proto/proto and @hashgraph/proto/com/hedera/mirror/api/proto. However, I am not convinced this is do-able, but if it is, it would solve your issue. You'd finally be able to do

import { AccountID } from "@hashgraph/proto/proto";

[janaakhterov]

Closing this issue, if we really want to try out modules or something else that's fancy we should create a separate issue.