use @ln-zap/proto-loader instead of @grpc/proto-loader

[bucko13]

The @grpc/proto-loader module that is currently used causes a problem when going through a build with webpack. The error returned when ln-service is built is:

The "path" argument must be of type string. Received type number

This can be an indication of node's path module operating on a dynamic import (since one of the arguments isn't a string when being analyzed by webpack). ln-zap appears to have a fork of this module that fixes the issue (probably because they are using proto-loader in an electron app which needs to be compiled with webpack). This PR replaces the @grpc package w/ Zap's.

Also fixed a typo in the readme.

[alexbosworth]

is there a repo for the ln-zap proto loader?

[bucko13]

Yep! Had to dig to find it since the packages are all bundled in another repo. Here's the root repo: https://github.com/LN-Zap/grpc-node

And the proto-loader repo: https://github.com/grpc/grpc-node/tree/master/packages/proto-loader

It looks like I was wrong about what I thought was causing the problem though as the use of dirname hasn't changed.

[alexbosworth]

Interesting, yeah I can't tell what the difference is

Could try switching to grpc-js instead of the grpc-node

[bucko13]

I found this one dependent by the same maintainer, lnd-grpc and here's the commit explaining the change: https://github.com/LN-Zap/node-lnd-grpc/commit/efc2ba243181fcdd94b711ed94c29ae048b30f46

fix: use forked grpc proto-loader that doesnt require google protos

[alexbosworth]

It's hard for me to figure out what is changed though

[bucko13]

Interesting, you can actually see the difference in the final build. @grpc/proto-loader has this line in its index.js file:

var sourceDir = path.join(path.dirname(require.resolve('protobufjs')), 'google', 'protobuf');

That's missing in the ln-zap version, which is actually 14 lines shorter (so obviously a bit more that's different). So that means I might've been right about what was causing the issues in my build, even if that's not what they might necessarily have been targeting! Maybe @mrfelton can give more details?

[mrfelton]

The issue here is due to a change introduced in https://github.com/grpc/grpc-node/pull/811. Specifically in this commit https://github.com/grpc/grpc-node/commit/cbff0b46012ce286e1b54c7a05128667b0d72aad

They are now including the google protos and attempting to load them in. However the way that they do the include is not compatible with a webpack/electron setup like ours sine they rely on dirname which is generally not set correctly in this type of setup.

Given that we have always worked with proto files that have the google includes commented out we opted for a simple fix to essentially just revert this change (see https://github.com/LN-Zap/grpc-node/commit/03549a0f8870c703c01b673caefc23e5e6bfecfe).

Another option would be to patch proto-loader to provide better support for these includes in which the user could specify where to load the proto files from. Or, to use webpack externals to exclude proto-loader from the webpack build in our codebase (no something we wanted to do).

[bucko13]

Ok yeah, same problem I was running into then! And it looks like you’re publishing off of this branch which is why we couldn’t pinpoint what was different

[alexbosworth]

How about instead of replacing the existing protoloader, flexibility is added to include an external protoloader?

This dirname thing seems like it could be fixed at the grpc repo

[bucko13]

We could, but I wonder what the chances are of them accepting it since this would probably be a breaking change. Also, looking back at the PR that @mrfelton referenced, it seemed like that was how they originally had it (to conditionally include the well-known protos) and explicitly made the decision to have them loaded all the time at runtime.

In any case, I'll file an issue on their repo pointing out the problem in the approach they took.

[bucko13]

Seems like we're not the only ones: https://github.com/grpc/grpc-node/issues/969