grpc issues with alpine linux, complete incompatibility with node 7.x.x

[AVVS]

Environment details

• OS: alpine linux
• Node.js version: 6.x.x, 7.x.x
• npm version: 3.10.x
• @google-cloud/storage version: <= 0.5.0

Steps to reproduce

This module relies on @google-cloud/common, which, in turn, relies on grpc. This causes problems for alpine linux, since it uses muslc. On 6.x.x it is possible to compile by providing glibc wrapper around muslc, so that compatible APIs are available, on node 7.x.x it just crashes during compilation (missing 7.x.x support)

Questions are:

a) why common depends on grpc? I haven't seen a single call using it in the sub-package
b) if I'm right with (a) - could somebody refactor @google-cloud/common and make grpc an optional dependency?
c) if (b) is doable, it's possible to load optional dependencies using Object.defineProperty on "demand", which, as a result, will allow google-cloud/storage API to be freely used on any system without any effort to maintain grpc module compatibility with anything

Thanks for considering this, looking forward to your feedback

[stephenplusplus]

My understanding of optionalDependency means if it can't install at run time, "that's okay; our code will work without it". That's not exactly the case for us. For the APIs that use GrpcService (a class in common), we absolutely need gRPC.

I haven't heard of the solution you mentioned in point number 3. Can you elaborate on that?

I would like to find a solution where our modules that don't need gRPC don't bother trying to install it. And we are actually working towards that, as we phase out GrpcService. For now, the best solution is getting this fixed in gRPC.

[AVVS]

basically the idea looks like this:

function demandProperty(obj, name, modPath) {
    Object.defineProperty(obj, name, {
        configurable: true,
        enumerable: true,
        get: function demandGetter() {
            const mod = require(modPath);
            Object.defineProperty(obj, name, {
                configurable: false,
                enumerable: true,
                value: mod
            });
            return mod;
        }
    });
}

// grpc module
demandProperty(exports, 'grpc', 'grpc');

So grpc module is not loaded when @google-cloud/common is required, instead it will only load if and only if any of the grpc functions are actually used and required.

So going back to making this an "optional" dependency - it will allow parts of the API that don't require grpc to work. However, if something that requires that API is used node will crash. But that is expected, of course.

[stephenplusplus]

The problem with that is runtime vs installation time warnings. If the user can't install gRPC, it should be big and bold so that they don't try to run code that doesn't have a chance.

You're definitely right that there is a problem, which is that we have a module (google-cloud/storage) that doesn't need grpc, yet we're failing the installation because it can't install something it doesn't need. I think the way around it is by getting rid of grpc from the common module entirely.

We're heading that way, as the APIs that do need gRPC (Bigtable, Datastore, more) are switched to use a dependency directly that wraps gRPC (google-gax). This means it won't need common for the GrpcService class it exposes, or the grpc dependency at all. I can't say for sure when we'll be there, but the overhead to come up with an intermediary solution is likely too much (having two separate modules; google-cloud/common and a google-cloud/common-grpc).

gRPC is working on offering better support for Node v7, which will remedy this issue in terms of the installation failure. And as mentioned earlier, the unnecessary installation part of this issue is being worked on as well.

[stephenplusplus]

@callmehiphop do you think we should side-load grpc? So common still exposes GrpcService, but doesn't declare the grpc dependency itself. Instead, datastore does, and provides it as an argument to the GrpcService constructor.

[ofrobots]

+1 on removing grpc dependency from common.

[callmehiphop]

@stephenplusplus I think that sounds fine to me. If we are shrinkwrapping our dependencies, couldn't we just wrap the grpc require in a try/catch and go based off that?

[stephenplusplus]

That's more like a runtime approach than an installation time, which offers more advantages (see #1822 (comment)). Let me know if I misunderstood.

[callmehiphop]

Ah, I see. I'm not a huge fan of the sideloading. Maybe we should just make a @google-cloud/grpc dependency that services like Datastore use and remove all the grpc related items from common.

[stephenplusplus]

I would rather see it named @google-cloud/common-grpc so there's no confusion between that utility package and the grpc library itself.

Here's why I think we should pursue grpc as an argument:

• @google-cloud/common-grpc will require @google-cloud/common as its own dependency, as GrpcService and GrpcServiceObject inherit from Service and ServiceObject. This means a minor bump in @google-cloud/common forces a minor bump in @google-cloud/common-grpc. That's more overhead and points of failure in the release cycle.
• This solution will be temporary. We will soon be able to phase out grpc from common when we incorporate google-gax, which has grpc as its own dependency. Having only a couple of files (GrpcService and GrpcServiceObject) in their own module will be inconvenient and awkward to work with. We'll inevitably want them back in the common utility module.

[callmehiphop]

I would rather see it named @google-cloud/common-grpc so there's no confusion between that utility package and the grpc library itself.

👍

This means a minor bump in @google-cloud/common forces a minor bump in @google-cloud/common-grpc. That's more overhead and points of failure in the release cycle

Seems like this could be remedied by doing something like "@google-cloud/common": ">=0.9.0" and only updating the version for breaking changes instead of additive changes.

This solution will be temporary. We will soon be able to phase out grpc from common when we incorporate google-gax

That is true, but it kinda feels like a moot point considering that the side-loading approach would also be a temporary fix.

Having only a couple of files (GrpcService and GrpcServiceObject) in their own module will be inconvenient and awkward to work with. We'll inevitably want them back in the common utility module.

Not sure I follow the logic here, most of our modules only contain a few files. @google-cloud/common for example is pretty small and will only get smaller once we have completely transitioned to google-gax.

[stephenplusplus]

Seems like this could be remedied by doing something like "@google-cloud/common": ">=0.9.0" and only updating the version for breaking changes instead of additive changes.

I don't think that's an ideal solution, because we might intentionally break something in common 0.10 that needs to be addressed by common-grpc before its users should upgrade.

That is true, but it kinda feels like a moot point considering that the side-loading approach would also be a temporary fix.

But it's a temporary fix with no lasting side effects. We won't have to maintain an additional module.

Not sure I follow the logic here, most of our modules only contain a few files. @google-cloud/common for example is pretty small and will only get smaller once we have completely transitioned to google-gax.

Yes, small modules are good, but small modules that have identical purposes and classes with identical methods are unnecessary and more difficult to work with.

What concerns you about using grpc as an argument? For a precedent, we accept promise as an argument, which is meant to be a module, right? retryRequest accepts a request argument so that it can use a custom transport library. I'm pretty sure gax is going to accept grpc as an argument as well, or at least it was brought up in a code example at one point.

[callmehiphop]

For a precedent, we accept promise as an argument, which is meant to be a module, right? retryRequest accepts a request argument so that it can use a custom transport library

IMO it's not really the same. These two allow the user to opt out of the default functionality in favor of a module with the same interface. What we are currently trying to solve is how to decouple grpc from modules that don't leverage it.

What concerns you about using grpc as an argument?

It's not elegant. It would work fine, but I can't help but feel like it's just an awkward way of handling grpc that we're willing to deal with because we think our hard dependency on it is on its way out.

IMO a new module provides a cleaner separation with more predictability.

[stephenplusplus]

There are some very undesirable side effects that we can avoid in the short period of time before gax is widely introduced. If the separation you're siding with is more important, let's move forward with it.