Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed fabric-samples/asset-transfer-basic/application-gateway-typescr… #1011

Closed
wants to merge 1 commit into from

Conversation

k3t4ngit
Copy link

Fixed fabric-samples/asset-transfer-basic/application-gateway-typescript grpc client type check

…ipt grpc lcient type check

Signed-off-by: k3t4ngit <ketantiwari@live.in>
@lehors
Copy link
Contributor

lehors commented Apr 20, 2023

Can you please explain what this actually fixes? Are you getting an error or warning?
I don't see any issues running this code (the original one) and we have this kind of code in a lot of other places in fabric-samples so if we need to fix it we should probably fix it throughout the repo.

@denyeart
Copy link
Contributor

@bestbeforetoday Can you take a look?

@k3t4ngit
Copy link
Author

The connect method from @hyperledger/fabric-gateway expects GrpcClient obj as parameter.
But the '@grpc/grpc-js' client is not of type GrpcClient.
I have to cast the type to compile.

Copy link
Member

@bestbeforetoday bestbeforetoday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change really should not be necessary, and is certainly not a desirable change. Typings should be correct rather than explicitly force potentially incompatible type conversions using unknown.

The fabric-gateway connect() function was changed to take a GrpcClient interface since changes to the private internals of the grpc-js Client class could break type compatibility if the application's version of grpc-js differed from the one used when packaging fabric-gateway. The interface describes only the public members of the grpc-js Client class to minimise these issues. See hyperledger/fabric-gateway#555.

It does seem that some of the grpc-js classes referred to in the GrpcClient interface can also suffer from version-to-version type compatibility issues between different versions of grpc-js. To minimise this problem, grpc-js was removed as a direct dependency of the sample application, and the application instead relies on exactly the version of grpc-js resolved as a dependency of fabric-gateway.

Running locally from a clean environment and using the latest state of the fabric-samples main branch, I don't see any issues. npm install and npm start both run cleanly for me without this change. Running npm ls @grpc/grpc-js gives me this output:

asset-transfer-basic@1.0.0
└─┬ @hyperledger/fabric-gateway@1.2.2
  ├── @grpc/grpc-js@1.8.14
  └─┬ @hyperledger/fabric-protos@0.2.0
    └── @grpc/grpc-js@1.8.14 deduped

If you do see typing issues, it would be good to understand how they have happened and with what package versions, not to just apply unsafe casts to the code.

@denyeart
Copy link
Contributor

denyeart commented Aug 9, 2023

Based on bestbeforetoday comments, I'll go ahead and close.

@denyeart denyeart closed this Aug 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants