Typescript compilation #78

Phillammon · 2020-11-27T18:06:51Z

What is the goal of this PR?

Compile protobuf files to typescript rather than raw javascript in the nodejs target.

What are the changes implemented in this PR?

Create genrule calling protoc compiler directly with the appropriate plugins to output both javascript and typescript type definition files when built.

Phillammon · 2020-11-27T18:07:34Z

.gitignore

 __pycache__
+
+# Node Modules #
+node_modules


Having added node_modules, we do not want to add it to git

Phillammon · 2020-11-27T18:08:08Z

WORKSPACE

-workspace(name = "graknlabs_protocol")
+workspace(
+    name = "graknlabs_protocol",
+    managed_directories = {"@npm": ["node_modules"]},


As we've added node modules, we want to be able to use the modules as targets, which necessitates managing it as a directory.

I feel like we should rename @npm to @node_modules for clarity - what do you think?

We would make the same change in client-nodejs.

Phillammon · 2020-11-27T18:09:18Z

WORKSPACE

+  name = "npm",
+  package_json = "//:package.json",
+  package_lock_json = "//:package-lock.json",
+)


Performing an npm_install is required in order to get said node modules up to date and in the build, and we need to set up the nodejs rules so that we can fetch them in. (The node modules specifically required are the protoc compiler and the gen_grpc plugin for it)

Phillammon · 2020-11-27T18:11:06Z

grpc/nodejs/BUILD

+        "protobuf/session_pb.d.ts",
+        "protobuf/session_pb.js",
+        "protobuf/transaction_pb.d.ts",
+        "protobuf/transaction_pb.js",


Set of outputs changed to reflect we a) now only have a single service and b) we now have both a .js and a .d.ts file for every incoming proto file.

Phillammon · 2020-11-27T18:27:49Z

grpc/nodejs/BUILD

+            --grpc_out='./$(@D)/' \
+            --ts_out='./$(@D)/' \
+            --proto_path=`dirname $(execpath //:WORKSPACE)` \
+            $(execpaths //protobuf:proto-raw-buffers);",


This command is a bit brave, but divided by line:

Symlinks the node runfiles required by the typescript plugin into the directory where the protocol compiler can see it

Runs the node protoc compiler

Using the protoc-gen-ts plugin to generate typescript type definitions in addition to the javascript definitions

Outputting the javascript, typescript, and service definitions to the output directory specified by bazel ($(@d))

Looking in the workspace for the relative paths to the .proto files

using the .proto files as specified in the proto-raw-buffers filegroup as the inputs

This is excellent documentation, nice work! I think we should add it to the code itself, in comments, so this doesn't get lost in the ether.

Phillammon · 2020-11-27T18:29:33Z

grpc/nodejs/BUILD

+            "@npm//google-protobuf",
+            "//protobuf:proto-raw-buffers",
+            "//:WORKSPACE"
+        ],


The required tools for the above command. Workspace only has to be included in order to find the relative path for the .proto files.

Again, let's add this as a comment in the code itself 🙂 Something like: "We use //:WORKSPACE to find the relative path for the .proto files".

Phillammon · 2020-11-27T18:30:47Z

grpc/nodejs/protocol.json

+    "skipLibCheck": true,                     /* Skip type checking of declaration files. */
+    "forceConsistentCasingInFileNames": true  /* Disallow inconsistently-cased references to the same file. */
+  }
+}


Standard tsconfig file generated automatically by tsc, for tsc.

Phillammon · 2020-11-27T18:31:14Z

package.json

+    "grpc-tools": "^1.9.1",
+    "grpc_tools_node_protoc_ts": "^5.0.1"
+  }
+}


Node dependencies in order to compile typescript

Do we need both package.json files?

Phillammon · 2020-11-27T18:32:51Z

protobuf/BUILD

+        "session.proto",
+        "transaction.proto",
+    ]
+)


The libraries we currently have pre-compile the .proto files, which is inconvenient as we wish to recompile them. Where we could (and in future should) build a full blown bazel rule extracting .src_file from the proto_library targets, for the time being providing a filegroup is equally legitimate

Totally with you on this one - let's raise a GitHub issue for it (in graknlabs/protocol) and possibly put a TODO in the source code linking to the issue so it doesn't get lost.

…ol into typescript-compilation

Phillammon · 2020-11-30T20:41:26Z

grpc/python/BUILD

 py_library(
    name = "protocol",
-    srcs = ["protocol_pkg"],
+    srcs = ["protocol-pkg"],


Renamed for consistency on the way past

Nice effort!

However, will a Bazel target with - rather than _ work in Python @vmax ?

Phillammon · 2020-11-30T20:41:54Z

grpc/nodejs/package.json

+  },
+  "devDependencies": {
+    "typescript": "3.9.7"
+  }


Package.json for compiling the npm package

alexjpwalker

Overall this looks excellent. I've highlighted a couple of trailing newlines and suggested that we add relevant documentation to GitHub and the source code where applicable.

.gitignore

alexjpwalker · 2020-12-01T09:31:03Z

WORKSPACE

-workspace(name = "graknlabs_protocol")
+workspace(
+    name = "graknlabs_protocol",
+    managed_directories = {"@npm": ["node_modules"]},


I feel like we should rename @npm to @node_modules for clarity - what do you think?

We would make the same change in client-nodejs.

alexjpwalker · 2020-12-01T09:32:45Z

grpc/nodejs/BUILD

+            --grpc_out='./$(@D)/' \
+            --ts_out='./$(@D)/' \
+            --proto_path=`dirname $(execpath //:WORKSPACE)` \
+            $(execpaths //protobuf:proto-raw-buffers);",


This is excellent documentation, nice work! I think we should add it to the code itself, in comments, so this doesn't get lost in the ether.

alexjpwalker · 2020-12-01T09:34:37Z

grpc/nodejs/BUILD

+            "@npm//google-protobuf",
+            "//protobuf:proto-raw-buffers",
+            "//:WORKSPACE"
+        ],


Again, let's add this as a comment in the code itself 🙂 Something like: "We use //:WORKSPACE to find the relative path for the .proto files".

alexjpwalker · 2020-12-01T09:35:05Z

grpc/nodejs/package.json

+  "devDependencies": {
+    "typescript": "3.9.7"
+  }
+}


Suggested change

}

}

alexjpwalker · 2020-12-01T09:36:47Z

package.json

+    "grpc-tools": "^1.9.1",
+    "grpc_tools_node_protoc_ts": "^5.0.1"
+  }
+}


Do we need both package.json files?

alexjpwalker · 2020-12-01T09:38:10Z

protobuf/BUILD

+        "session.proto",
+        "transaction.proto",
+    ]
+)


Totally with you on this one - let's raise a GitHub issue for it (in graknlabs/protocol) and possibly put a TODO in the source code linking to the issue so it doesn't get lost.

lolski · 2020-12-01T09:34:50Z

.grabl/automation.yml

+          export DEPLOY_NPM_PASSWORD=$REPO_GRAKN_PASSWORD
+          export DEPLOY_NPM_EMAIL=$REPO_GRAKN_EMAIL
+          bazel run --define version=$(git rev-parse HEAD) //grpc/nodejs:deploy-npm -- snapshot
+        dependencies: [build, build-dependency, test]


We don't have a job test do we? In that case this should be removed

lolski · 2020-12-01T09:37:51Z

WORKSPACE

+#########################
+
+load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
+http_archive(


Can we update the version of rules_nodejs in @graknlabs_dependencies with this one and then have the WORKSPACE file load from there instead? You can get @alexjpwalker 's help for the know-how.

lolski · 2020-12-01T09:40:16Z

package.json

@@ -0,0 +1,11 @@
+{
+  "name": "protocol",
+  "version": "1.0.0",


VERSION should match the version file shouldn't it? In that case, 2.0.0-alpha?

lolski · 2020-12-01T09:41:43Z

grpc/python/BUILD

 py_library(
    name = "protocol",
-    srcs = ["protocol_pkg"],
+    srcs = ["protocol-pkg"],


Nice effort!

However, will a Bazel target with - rather than _ work in Python @vmax ?

Co-authored-by: Alex Walker <alexjpwalker@gmail.com>

alexjpwalker · 2020-12-01T16:58:50Z

dependencies/graknlabs/repositories.bzl

        name = "graknlabs_dependencies",
        remote = "https://github.com/Phillammon/dependencies",
-        commit = "8526f49ce77fcd8e9029ab4b08d0fd5d0b4497f8", # sync-marker: do not remove this comment, this is used for sync-dependencies by @graknlabs_dependencies
+        commit = "63bfceddc8493e743f54ab230239cd42eb1bccdf", # sync-marker: do not remove this comment, this is used for sync-dependencies by @graknlabs_dependencies


Update this before merging

alexjpwalker

Approving - just remember to bump dependencies and verify that it builds in Grabl

Alistair Crook and others added 8 commits November 25, 2020 20:16

Intermediary broken commit

5e8032d

return outputs explicitly

3d57b19

Intermediary push

8d39412

Functioning typescript compilation

15f98e4

Finalise ts_librar, standardize casing

8607ec3

Include ts_project

fa350ce

Correctly export protocol

efccaea

Generate grakn service

a3257a5

Phillammon commented Nov 27, 2020

View reviewed changes

Remove unused import

c58c23d

Phillammon commented Nov 27, 2020

View reviewed changes

Alex Walker and others added 10 commits November 28, 2020 12:40

Lock down all dependencies to specific versions

75f6fc7

Merge branch 'typescript-compilation' of github.com:Phillammon/protoc…

e3dbd71

…ol into typescript-compilation

Generate @grpc-js-compatible _pb files

3d94758

Enable building npm package

752402d

Attempted snapshot upload

6885d56

Make node package deployable so client-node can consume

a4b7af9

Remove top level main/types

5297990

Remove module type

9701a01

Stop preserving symlinks

0324ea0

Add grabl automation

a7e644a

Phillammon requested review from alexjpwalker, haikalpribadi and lolski and removed request for alexjpwalker and lolski November 30, 2020 20:40

Phillammon commented Nov 30, 2020

View reviewed changes

grpc/nodejs/package.json

},

"devDependencies": {

"typescript": "3.9.7"

}

Copy link

Author

Phillammon Nov 30, 2020

Choose a reason for hiding this comment

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

Package.json for compiling the npm package

Phillammon requested a review from lolski November 30, 2020 20:43

alexjpwalker requested changes Dec 1, 2020

View reviewed changes

lolski reviewed Dec 1, 2020

View reviewed changes

lolski added the type: feature label Dec 1, 2020

Alistair Crook and others added 2 commits December 1, 2020 10:07

Update .gitignore with trailing newline

18a1883

Co-authored-by: Alex Walker <alexjpwalker@gmail.com>

Fixes per code review

ee8b0ac

Phillammon requested a review from alexjpwalker December 1, 2020 12:28

Alistair Crook added 13 commits December 1, 2020 12:29

Merge branch 'master' into typescript-compilation

0a20a7b

Revert python changes

5c04300

Remove RBE from build

690c3e7

Insert debug echoes

97f7b47

Insert nobazel_node_patches

424b54c

Correctly apply nobazel

83314bf

Re-add symlink preservation?

1162848

Remove node dependency

af3f783

Really dodgy dependencies patch

40cbdb2

Remove plugin binary

e042c34

readd plugin bin

02e66ae

Re-set no node patch for plugin

7eb4b55

Replace package.json

321c95f

alexjpwalker reviewed Dec 1, 2020

View reviewed changes

alexjpwalker approved these changes Dec 1, 2020

View reviewed changes

Alistair Crook added 6 commits December 1, 2020 16:59

Point back at regular dependencies

af8e6e0

exclude node modules from checkstyle checks

76e7d2d

exclude node modules PROPERLY

d5b3a2d

Refine checkstyle test

e4869c5

Use new checkstyle tests from dependencies

c3f85f9

Exclude package and package-lock from checkstyle

c94616a

lolski approved these changes Dec 1, 2020

View reviewed changes

lolski merged commit e161ae8 into typedb:master Dec 1, 2020

Typescript compilation #78

Typescript compilation #78

Uh oh!

Conversation

Phillammon commented Nov 27, 2020

What is the goal of this PR?

What are the changes implemented in this PR?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexjpwalker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexjpwalker left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants