grpcweb npm runtime module

[zaucy]

Created for #197

This sets up an npm module that runs the google closure compiler on the client related js before publishing to npm.

[stanley-cheung]

Doesn't really need to. These 2 files are irrelevant for this purpose.

[zaucy]

Wouldn't grpc.web.GatewayClientBase be required if a client was compiled with mode=base64?

[stanley-cheung]

We may have to remove it for some other reasons in the near future. So I just don't want to call this out as a TODO. We can add support for that any time.

[stanley-cheung]

Is the package-lock.json necessary to be checked in? It seems to be huge. Is it all just pulled in by the google-closure-compiler dependency (and then recursive for other node_modules)?

[stanley-cheung]

Should package.json be on the root directory of the repo instead?

.npmignore can be put in the root directory as well.

.gitignore can merge with the one in root directory (seems to be introduced by 3 PRs at the same time :P)

script/build.js can be put into the existing scripts directory

I am not sure if the npm/ folder is a convention?

[zaucy]

Typically it would be included in a project to keep its runtime dependencies from changing between installs, but in this case it actually is unncessary. There are no runtime dependencies, just the google-closure-compiler dev dependency. It was really just force of habit. I'll remove it.

[stanley-cheung]

Btw, index.js has not been checked in?

[zaucy]

Btw, index.js has not been checked in?

It gets generated on an npm publish / npm run build

I am not sure if the npm/ folder is a convention?

I don't believe it is. I just didn't want to pollute the root directory. If there are multiple npm modules that may live in this repository it might be good to have subfolders for them.

Do you want me to move it?

[stanley-cheung]

Ah. Can you add some notes to the main README.md to show how a typical npm workflow goes if a user wants to use grpc-web thru that route? Perhaps a Quickstart with npm section or something like that.

Thanks for your contribution! This is very important.

[stanley-cheung]

Do you want me to move it?

Yes please. Let's not create a npm/ folder for now. We can do it from the root as there's only 1 package for now.

[zaucy]

It would mean that all the other files/folders will need to be added to the .npmignore or else they would be included with a publish. Is that okay?

I could also make a packages folder similar to how https://github.com/grpc/grpc-node does it.

[lalomartins]

Please make sure to include the Readme (or a Readme) in the package so that it gets displayed on npmjs.com; looking at your test package at the moment gives me “Unable to find a readme“ which is not ideal.

[stanley-cheung]

It would mean that all the other files/folders will need to be added to the .npmignore or else they would be included with a publish. Is that okay?

Yes please add .npmignore to all the other folders. Thanks.

[stanley-cheung]

Let's call it version 0.1.0 please. Thanks.

[stanley-cheung]

It would mean that all the other files/folders will need to be added to the .npmignore or else they would be included with a publish. Is that okay?

Yes please add .npmignore to all the other folders. Thanks.

Now that I think about it more. Should we put package.json and .npmignore only at the javascript/net/grpc/web directory?

This repo contains both the gRPC-Web client JS code and the custom Nginx proxy code. package.json obviously doesn't apply to the latter. So it looks like package.json belong better at javascript/net/grpc/web/ folder?

Sorry for going back and forth. Not very familiar with how github repo and npm modules interact

[lalomartins]

@stanley-cheung Normally they correspond one-to-one. But in cases where they don't, especially the “monorepo” pattern, indeed package.json and friends go in the inner directory (in the monorepo case, packages/foo). So that would be a +1 from me, but it would still be preferable to somehow include the Readme, and that would require some extra gymnastics in this case.

[zaucy]

Is javascript/net/grpc/web/ the final decision?

I'm more in favour of packages/grpc-web myself. I could see another npm package in this repository that wraps the protoc plugin in an npm module which could reside in packages/protoc-gen-grpc-web.

It may make sense to maintain separate README's for the npm module instead of using the main repository one. The quick start using docker-compose for instance could be omitted as well as any closure related examples. A project that is using the npm module most likely doesn't need to see that information first hand while viewing the package at npmjs.com, yarnpkg.com etc.

[stanley-cheung]

+1 to packages/grpc-web. This is how grpc/grpc-node repo does it - they have multiple npm packages under the packages/ folder.

[lalomartins]

Well I'm not a contributor, I'm not CNCF and not Google, so anything I say is from the perspective of an user with long years of JS experience.

From that perspective, I'd also prefer packages/grpc-web. It's more intuitive and more in line with the community standards. But, in that case the correct thing to do would be to move the actual sources to that location, and that would mean changing the whole build process for the Closure version as well. I'm not sure how the maintainers would like that.

[stanley-cheung]

I am open to moving all the JS source code to be under packages/grpc-web. The code being in javascript/net/grpc/web was purely because we started developing code for internal use case first. This should have no bearing for the OSS/Github use case.

@zaucy If you feel like moving all the source code is too big a task, feel free to leave the PR in the current state and I can do the big move on top of that.

[stanley-cheung]

Actually we just need to rename javascript/net/grpc/web to packages/grpc-web?

[zaucy]

I kept the scripts folder for the grpc-web module in packages/grpc-web/scripts just because the script is only related to the build step when running npm run build.

Actually we just need to rename javascript/net/grpc/web to packages/grpc-web?

I'll let you move the js files in case there are any issues with the examples / closure related pieces that I haven't touched on. All that would need to be changed in the grpc-web package is this line https://github.com/grpc/grpc-web/pull/210/files#diff-dabf4379a467e2218cfdb99795a1558bR9

[zaucy]

Is packages/grpc-web/README.md okay? I just took the top part of the main README and added a quick start using protoc.

[stanley-cheung]

Is packages/grpc-web/README.md okay? I just took the top part of the main README and added a quick start using protoc.

Yep SGTM.

@zaucy Thanks for all the great work! Now all the changes are in the new packages/grpc-web folder. Very clean. I will move the rest of the source code later (and make sure all the other dockerfiles and stuff are matched).

[stanley-cheung]

@zaucy Can you squash the commits please?

[zaucy]

@stanley-cheung Done!

[stanley-cheung]

[Edit] Nevermind. My node version is too old. Upgraded to Node 8 and error went away.

[zaucy]

Ah the script is not compatible with your version of node. Is it required that it work with node 4? I'm running node 8 which is the current LTS.

[lalomartins]

Node 4 is way past EOL. The earliest supported release is 6.

I guess this is an use case for the engines field in package.json?

[zaucy]

I guess this is an use case for the engines field in package.json?

That would be unnecessary. The error is caused by the build script. The engines field would be used for end users installing the package.

[stanley-cheung]

Great! This all works. I ran npm run build from the packages/grpc-web directory and I see that the index.js file got compiled by the closure compiler.

[stanley-cheung]

Nevermind, I didn't realize I was in a VM. We don't have to support Node 4 at this stage.