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

integrate client-nodejs with bazel build system #16

Closed
wants to merge 1 commit into from
Closed

integrate client-nodejs with bazel build system #16

wants to merge 1 commit into from

Conversation

vmax
Copy link

@vmax vmax commented Oct 19, 2018

Why is this PR needed?

So we are able to test client-nodejs with bazel

What does the PR do?

Migrates client-nodejs to bazel

Does it break backwards compatibility?

no

List of future improvements not on this PR

publishing artifacts to npm

Implementation details: short version

We need to have correct imports in generated *_pb.js files and then we need to put generated files to same locations they would have been in if compiled by npm

Implementation details: extended version

  • _proto_gen genrules take proto files and strip protocol/session import statements using generate_js.sh
  • generated_node_lib is generated from those protos (a set of *_pb.js and *_grpc_pb.js files). A modified version of grpc/grpc is used so file is generated even if there are no services in files
  • generated_node_lib_transformed moves generates js files to appropriate locations so require() statements in client's sources are satisfied
  • grakn combines all js files into one package
  • grakn_dummy_binary is needed to generate a tar.gz bundle with all dependencies
  • jest_tests runs tests with jest. We need to use .tar.gz bundle because jest cannot handle symlinks bazel is producing relevant issue and they refuse to merge PR that fixes it. This method resembles what has been done for client-python (we generate zip module there)

@haikalpribadi haikalpribadi self-requested a review October 19, 2018 15:45
Copy link
Owner

@haikalpribadi haikalpribadi left a comment

Choose a reason for hiding this comment

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

This PR adds 8K lines of package-lock.json, @vmax. I think we should remove it.

Copy link
Owner

@haikalpribadi haikalpribadi left a comment

Choose a reason for hiding this comment

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

and why do we need lots of manual genrules in client-nodejs/BUILD, @vmax ?

@vmax
Copy link
Author

vmax commented Oct 26, 2018

Replaced with #25

@vmax vmax closed this Oct 26, 2018
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.

2 participants