hyperledger / iroha-javascript

JavaScript library for Iroha, a Distributed Ledger Technology (blockchain) platform.
https://wiki.hyperledger.org/display/iroha
Apache License 2.0
94 stars 64 forks source link

Move grpc from devDependencies to dependencies #173

Closed outSH closed 10 months ago

outSH commented 11 months ago

grpc is a dependency of iroha-helpers (irohaV1) but it's listed under devDependencies instead of dependencies. This causes error when user doesn't have grpc installed. Issue occured at our project (Hyperledger Cacti) when we updated from deprecated grpc to grpc-js.

To reproduce initialize empty TS nodejs project, npm i iroha-helpers (only), compile the following:

import { QueryService_v1Client as QueryService } from "iroha-helpers/lib/proto/endpoint_grpc_pb";

console.log(QueryService.toString());

And run. Error printed:

Error: Cannot find module 'grpc'
Require stack:
- /home/vagrant/tmp/iroha-check/node_modules/iroha-helpers/lib/proto/endpoint_grpc_pb.js
- /home/vagrant/tmp/iroha-check/index.js
    at Function.Module._resolveFilename (node:internal/modules/cjs/loader:995:15)
    at Function.Module._load (node:internal/modules/cjs/loader:841:27)
    at Module.require (node:internal/modules/cjs/loader:1067:19)
    at require (node:internal/modules/cjs/helpers:103:18)
    at Object.<anonymous> (/home/vagrant/tmp/iroha-check/node_modules/iroha-helpers/lib/proto/endpoint_grpc_pb.js:9:12)
    at Module._compile (node:internal/modules/cjs/loader:1165:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1219:10)
    at Module.load (node:internal/modules/cjs/loader:1043:32)
    at Function.Module._load (node:internal/modules/cjs/loader:878:12)
    at Module.require (node:internal/modules/cjs/loader:1067:19) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/home/vagrant/tmp/iroha-check/node_modules/iroha-helpers/lib/proto/endpoint_grpc_pb.js',
    '/home/vagrant/tmp/iroha-check/index.js'
  ]
}
petermetz commented 11 months ago

I recommend migrating to the new, purely JS implementation instead because the legacy grpc package has been deprecated for a long time now:

image

outSH commented 11 months ago

@6r1d @0x009922 Any updates regarding this? It's blocking our CI at the moment. Let us know if this package is still supported and can have grpc fixed / updated in the nearest future. If it's deprecated (or is about to be), then we could remove support for Iroha V1 from cacti.

0x009922 commented 10 months ago

Hi @outSH I cannot help you with the functionality related to Iroha 1, unfortunately.

@6r1d, what about support of Iroha 1 in cacti?

6r1d commented 10 months ago

@outSH I will update you as soon as I get the decision on that matter.

outSH commented 10 months ago

@6r1d Thanks for looking into that, any news so far?

6r1d commented 10 months ago

@outSH, I'm still looking for someone who could help.

petermetz commented 10 months ago

@6r1d This might help a little bit (but do let me know if it doesn't). The migration from grpc to grpc/grpc-js is meant to be fairly straightforward. What I mean by that is that when I was migrating parts of the Cacti codebase the same way, 99% of the time the only thing I needed to do was to replace the package imports in the code and everything would just work fine out of the box afterwards. There was no need to refactor the code and risk introducing new bugs, etc.

petermetz commented 10 months ago

@6r1d I just ended up sending in a quick PR. It's a pretty simple change. I verified that it works with our Cacti Iroha connector tests. See my PR: https://github.com/hyperledger/iroha-javascript/pull/182

0x009922 commented 10 months ago

TBH, I am not sure why this error occurs in runtime, because grpc library is not imported from what I can observe in source code.

However, grpc is imported in a .d.ts file, which means it must be a production dependency, not a dev one.

With this in mind, I've approved #174.

upd: I am blind, just saw #182, nevermind.

outSH commented 9 months ago

@6r1d @0x009922 Hi, do you plan to release a version containing this fix?

outSH commented 7 months ago

@6r1d @0x009922 any news regarding releasing new version with that fix? :)

0x009922 commented 7 months ago

@outSH, created a task for it: #189