TINKERPOP-1489 JavaScript GLV #695

jorgebay · 2017-08-17T15:40:42Z

https://issues.apache.org/jira/browse/TINKERPOP-1489

Submitting the JavaScript for review to merge into tp32 after 3.2.6 code freeze and release.

Changes since #626:

Rebased.
Adapted code generation to use groovy templates (similar to TINKERPOP-1618 Removed dependence on groovy for gremlin-python #608 for Python).

jorgebay · 2017-08-29T10:27:29Z

VOTE +1

mvn clean install -DskipIntegrationTests=false passes.

API summary:

All methods are generated using groovy template files.
Naming conventions for js are very similar to java, so no significant changes there from the java one (except in() and from()` which are reserved keywords).
toList() and next() are used, in the same way as the rest of the GLVs, except that it causes async execution (no blocking API is provided, as it won't be usable in Node.js): next() returns an async iterator and toList() a promise that gets fulfilled with an Array.

Usage samples:

const vertices = await g.V().toList();

for await (const vertex of g.V()) {
  console.log(vertex.label);
}

spmallette · 2017-09-01T15:22:23Z

Hi @jorgebay - sorry - i just haven't had time to focus on this yet. I'll try to dig into it next week. Thanks for jamming on this and getting it ready for merging.

spmallette · 2017-09-05T16:42:23Z

I rebased this today and added the nashorn scriptengine implementation for GremlinScriptEngine. I'd like to get us running tests with js the way we do with python since it's jsr223 compliant. I guess I'll keep working toward that goal this week - i'd propose to hold-off on merging this until that is done.

jorgebay · 2017-09-06T08:37:53Z

Great! It would be really nice to have a working GremlinJavaScriptScriptEngine!

spmallette · 2017-09-07T18:59:52Z

I think the need to figure out the language agnostic way to test GLVs is becoming more important. I was pretty close to getting nashorn to execute gremlin-javascript natively, but there's weird pathing issue and npm integration issues that just aren't making it easy (possible). I tried to hack my way around the pathing problems and while i was making progress it was just getting ugly. I'll need to think about the language agnostic approach a bit before I worry about trying to go any further in this direction.

jorgebay · 2017-09-08T07:36:27Z

I've removed support for Nashorn as part of #626 (see first comment). JavaScript engines don't provide a standard way to deal with and import modules, so supporting with multiple runtimes is usually a pain (unlike other languages like Python that has the import system built-in).

If to support GremlinJavaScriptScriptEngine we need to support Nashorn on the GLV, we should try to split the GLV and the JS script engine in 2 separate tickets and try to tackle those separately.

Having a JavaScript GLV would enable users to write their traversals in js, making it easier for users from the js community to use Gremlin. On the other side, supporting js lambdas is a more marginal use case.

I think the need to figure out the language agnostic way to test GLVs is becoming more important

I think testing a GLV (bytecode generation, websocket connection, type mapping, ...) can't be implemented in a runtime agnostic way because the whole concepts are defined by the runtime. I think we can define a set of behaviours that the GLV must adhere. We can define a set of given-when-then statements (in doc), which we can make sure each GLV implements in actual tests.

On the other hand, I think testing a script engine can be generalized into a common suite (like ScriptEngineLambdaTest) plus specialized behaviour tests per language.

spmallette · 2017-09-08T10:43:40Z

I mostly cared about nashorn to achieve testing as we did in python. I sense that most folks won't want to run on nashorn itself, so perhaps direct support of nashorn doesn't really matter especially if we come up with a better way to test. I'd rather not complicate gremlin-javascript with multiple runtimes - GLVs require enough effort to support as it is.

btw, i felt like this lib got me pretty far in making npm work on nashorn:

https://github.com/nodyn/jvm-npm

wasn't too bad, but got tripped up on the pathing as it didn't seem to want to run in a context that didn't have the js files in the same root directory as where you were running the scriptengine from. I couldn't quite figure out how to make that work. Again, it probably doesn't make sense to chase that angle anymore though. I'd rather just get testing figured out for GLVs in general.

We can define a set of given-when-then statements (in doc), which we can make sure each GLV implements in actual tests.

if we can write them in a doc, then it would seem we could write them as cucumber tests ( https://cucumber.io/ ) that could actually be executed somehow. that's been the general plan i've had anyway. if you have any other ideas, please let me know - i'll probably start playing around with this idea today or get started on it fresh next week.

spmallette · 2017-09-25T12:27:39Z

Just a note for those following this ticket's progress. I'm in the process of doing TINKERPOP-1784 which will add a language agnostic test suite using gherkin defined feature files. That should allow us to test all GLVs in a nice way.

jorgebay · 2017-11-30T15:25:31Z

I've implemented the support files for the gherkin test suite.
Thanks to the test suite, I've found and fixed some bugs that were part of the original implementation.

mvn clean install -pl :gremlin-javascript -DskipIntegrationTests=false run the mocha based tests and the cucumber-based tests. I've included it on TravisCI also.

328 scenarios (22 skipped, 306 passed)

It's ready to be reviewed!

I would like the JavaScript GLV to be part of the next release cycle (3.2.8 / 3.3.2). This GLV have been sidelined several times since the original pull request #450 (Oct 2016!)...

spmallette · 2017-11-30T15:30:57Z

this is excellent. i agree that we should look to merge this for 3.2.8/3.3.2. as we hopefully release 3.2.7/3.3.1 we should see gremlin-js come into an official state in the first few months of 2018.

jorgebay · 2018-01-04T11:12:13Z

What a great time of the year to review this pr! 😄

spmallette · 2018-01-04T15:13:03Z

gremlin-javascript/src/main/javascript/gremlin-javascript/package.json

@@ -0,0 +1,39 @@
+{
+  "name": "gremlin-javascript",
+  "version": "3.2.7-alpha1",


should be 3.2.8-alpha1 - at this point. though, we've been using "release candidate" suffix for other GLVs as in "3.2.8-rc1" - perhaps we should continue in that vein?

I've used alpha to avoid messing up in case it was accidentally pushed while testing the pull request.
The version suffix is added in the determineVersion() method of the generation.groovy file.

spmallette · 2018-01-04T15:15:33Z

gremlin-javascript/pom.xml

+    <parent>
+        <groupId>org.apache.tinkerpop</groupId>
+        <artifactId>tinkerpop</artifactId>
+        <version>3.2.7-SNAPSHOT</version>


should be 3.2.8-SNAPSHOT at this point

spmallette · 2018-01-04T15:17:48Z

gremlin-javascript/pom.xml

+                        </goals>
+                        <configuration>
+                            <scripts>
+                                <script><![CDATA[


can you please move embedded scripts to files as we did with the other GLVs?

https://github.com/apache/tinkerpop/blob/tp32/gremlin-python/pom.xml#L483

Makes the pom a bit easier to read.

spmallette · 2018-01-04T15:21:54Z

gremlin-javascript/pom.xml

+                <activeByDefault>false</activeByDefault>
+            </activation>
+            <build>
+                <plugins>


is there any special configuration required in the release manager's local environment for this to work? Either way, It would be a massive help if you could provide a "Javascript Environment" section in the dev docs here:

http://tinkerpop.apache.org/docs/current/dev/developer/#system-configuration

jorgebay · 2018-01-08T11:18:43Z

I've addressed the comments made by @spmallette:

I've included sections on the development-environment.asciidoc file for js environment and info for the release managers.
Moved generation to a groovy file.

spmallette · 2018-01-09T22:34:24Z

gremlin-javascript seems to build ok for me locally, but not on docker. claims it needs a maven upgrade to 3.1.0 for the npm plugin to work. do you see the same thing @jorgebay ?

jorgebay · 2018-01-10T11:27:05Z

I'm getting the same failure, but I don't understand where the requirement to a newer maven is coming from within frontend-maven-plugin.

jbmusso · 2018-01-11T22:51:27Z

Quick update - I plan to check this PR this weekend.

jorgebay · 2018-01-16T14:26:29Z

I've just noticed that there isn't a g:T deserializer, I'll add it in the next days.

jorgebay · 2018-01-17T15:18:53Z

Thanks @dkuppitz for looking into the maven issue!

I'll rebase it and add a g:T deserializer.

jbmusso · 2018-01-17T23:17:55Z

Finally found some time. Wew.

Well, this PR is very well crafted - well done! My comments are only minor:

About utils.toPromise - if I understand you well, you want a dual callback/promise API for most async functions?
In graph-traversal.js, most methods of GraphTraversalSource and GraphTraversal and function attached to statics could maybe be dynamically created from an array of method names and dynamically added to these classes/object. I don’t know what would be the performance implication of this, but I don’t think there should be any unless V8 really can’t figure out what’s going on when parsing that file. Hopefully it's smart and figures out that the class is not changing. That’d lower the file size and help maintainability a lot.
ES6, most likely friendlier for more recent versions of V8 and supported by Node.js v4+ (see Node green):
- arguments is deprecated and is replaced by ...args for variadic functions
- most function keywords could be replaced by arrow functions (lexical scoping and/or concise syntax). I tend to keep function for top-level functions, and use fat arrows everywhere even when this binding isn't needed (ie. callback w/o this)
- array.splice(0) could be replaced by const copy = [...original]
- func.apply(null, arguments) could be replaced by func(...args) when first argument this value is indeed meant to be null
package.json: ./node_modules/.bin is added to the $PATH by npm or yarn, so we could just use "test": "mocha test/unit test/integration -t 5000". Yay npm!

I can fork and push 4 distinct commits for this if you want, so this can be cherry-picked.

A more major update would be to author in ES6/7/8 and add a transpilation step, so all runtime could use code that they can optimize best. Using babel with babel-preset-env combined with postinstall-build is an option. This will ensure that latest versions of Node.js use mostly non-transpiled code, while older versions automatically transpile what is strictly needed. That would make things more performant for latest versions of Node.js, since V8 optimizes a lot for new syntax, while still making the GLV compatible for older versions of Node.js. The nice thing is that the build step is automatically handled at install time by npm, so no extra maven coding should be required. I think such approach could be added in future releases and is out of scope today.

Also, I'm ok to transfer/donate the "gremlin" package name to Apache TinkerPop so this can be published under this name.

jorgebay · 2018-01-18T11:05:00Z

About utils.toPromise - if I understand you well, you want a dual callback/promise API for most async functions?

Just promise-based API, no callback-based API. utils.toPromise() its a way to support different "promise factories", the way promises are created in the driver. It's an idea to support any Promise API (like bluebird), but it's not really needed... maybe adds unnecessary complexity to the code? Do you think it makes more sense to remove it?

In graph-traversal.js, most methods of GraphTraversalSource and GraphTraversal and function attached to statics [...]

I like the idea of having the methods defined in code to support code completion on IDEs like VS Code and WebStorm, given that it's hard to remember all the traversal methods by heart. Maybe we can reduce the size and complexity of the generated code by using something like:

Instead of (current):

const statics = {};
statics.V = function (args) {
  const g = new GraphTraversal(null, null, new Bytecode());
  return g.V.apply(g, arguments);
};
statics.addE = function (args) {
  const g = new GraphTraversal(null, null, new Bytecode());
  return g.addE.apply(g, arguments);
};

Use:

const statics = {
  V: (...args) => callOnEmptyTraversal('V', args),
  addE: (...args) => callOnEmptyTraversal('addE', args),
  // ...
};

wdyt?

I'll start working on the other suggestions (ES6).

spmallette · 2018-01-18T23:45:38Z

I'm posting this to all open PRs of current relevance - this PR needs to be rebased against the branch it is targeted against given a broken python dependency that was published to pypi a day or so ago. I've pushed a fix on tp32 and master at this point and David Brown is working on getting an issue raised with the project that initiated the problem.

- Use class definitions - Use const and let - Computed property names in object literals

jorgebay · 2018-01-19T08:31:03Z

Rebased.

jbmusso · 2018-01-19T14:08:50Z

According to petkaantonov/bluebird#1026, users should be able to just patch the global Promise object in their application with:

global.Promise = require("bluebird");

I am unsure about other Promise libraries but I believe this approach should work as long as they're Promise/A+ standard compliant. Maybe we could also give it more thoughts and see for other ways to handle this in a future release, but I think it's worth making the code simpler at this point. I also feel it can/should be done at the application level.

For Traversal methods, I didn't think about IDEs and you're absolutely right about code completion. I think your proposed approach with callOnEmptyTraversal works best. This makes me think that I'm pretty sure people will want typed traversals with TypeScript or FlowType soon (thinking about https://github.com/DefinitelyTyped/DefinitelyTyped here for example).

- Use rest parameters - Use a more compact declaration for methods on statics - Bump engine to Node.js >= 6

jorgebay · 2018-01-22T14:32:58Z

I've pushed 2 commits to address the issues highlighted in the review.

I got rid of the promise factory and I'm using rest parameters and spread syntax (instead of parseArgs()) for GraphTraversal and GraphTraversalSource methods.

In order to ease up maintenance of the GLV, I've bumped the min version supported of the runtime to Node.js 6, as Node.js 4 is reaching EOL in April and it didn't support several ES2015 features (rest parameters being one of them).

jorgebay · 2018-01-23T09:06:05Z

I've updated the travis commands to include a mvn clean install -DskipTests before running GLV tests, GLV jobs take now 7 and 5 minutes respectively which is still not much.

jbmusso · 2018-01-23T15:13:49Z

All good to me!

VOTE +1

spmallette · 2018-01-24T18:00:34Z

All tests pass with docker/build.sh -t -n -i

VOTE +1

asfgit force-pushed the TINKERPOP-1489 branch from 043be33 to 1f00b7c Compare September 5, 2017 14:16

asfgit force-pushed the TINKERPOP-1489 branch 2 times, most recently from 6a0193f to fda5ab6 Compare November 23, 2017 09:20

asfgit force-pushed the TINKERPOP-1489 branch from 2173242 to f4a27b0 Compare November 30, 2017 13:47

spmallette reviewed Jan 4, 2018

View reviewed changes

asfgit force-pushed the TINKERPOP-1489 branch from db9c704 to e85385a Compare January 8, 2018 10:37

asfgit force-pushed the TINKERPOP-1489 branch from ee8ac38 to bf43cdc Compare January 17, 2018 15:52

jorgebay and others added 15 commits January 19, 2018 09:30

Traversal iterate() method

219ead0

Parse maps and mark scenarios with lambdas as skipped

9339a83

Fix Bytecode cloning

b9ca2ca

Vertex and Edge serialization

e95b8ea

Path assertions and feature step fixes

b8eb6c0

Support ignore scenarios

f9f7033

Add toLong() method

93cf86d

Support multiple mime types

b7f85ca

Use ES6 syntax

d458f76

- Use class definitions - Use const and let - Computed property names in object literals

Adjust pom.xml to be forward compatible

843a727

Move generation script to groovy file

2afd971

Include information of Js development env and release

8d722a9

Make Travis use a newer version of Maven.

397393d

install Maven 3.3.9 as part of the Docker build process

3553ea9

Include T deserializer

f2f9729

asfgit force-pushed the TINKERPOP-1489 branch from bf43cdc to f2f9729 Compare January 19, 2018 08:30

Remove utils.toPromise()

af718f7

asfgit force-pushed the TINKERPOP-1489 branch from 5fda818 to 154878e Compare January 22, 2018 14:10

Use rest parameters and compact syntax for statics

d37a3d8

- Use rest parameters - Use a more compact declaration for methods on statics - Bump engine to Node.js >= 6

asfgit force-pushed the TINKERPOP-1489 branch from 154878e to d37a3d8 Compare January 22, 2018 14:31

Travis: Use mvn install before running GLV tests

328c936

asfgit merged commit 328c936 into tp32 Jan 25, 2018

asfgit deleted the TINKERPOP-1489 branch January 25, 2018 13:18

jbmusso mentioned this pull request Apr 5, 2018

Error: Expected Stream, got object jbmusso/gremlin-javascript#91

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TINKERPOP-1489 JavaScript GLV #695

TINKERPOP-1489 JavaScript GLV #695

jorgebay commented Aug 17, 2017

jorgebay commented Aug 29, 2017

spmallette commented Sep 1, 2017

spmallette commented Sep 5, 2017

jorgebay commented Sep 6, 2017

spmallette commented Sep 7, 2017

jorgebay commented Sep 8, 2017

spmallette commented Sep 8, 2017

spmallette commented Sep 25, 2017

jorgebay commented Nov 30, 2017

spmallette commented Nov 30, 2017

jorgebay commented Jan 4, 2018

spmallette Jan 4, 2018

jorgebay Jan 8, 2018

spmallette Jan 4, 2018

spmallette Jan 4, 2018

spmallette Jan 4, 2018

jorgebay commented Jan 8, 2018

spmallette commented Jan 9, 2018

jorgebay commented Jan 10, 2018

jbmusso commented Jan 11, 2018

jorgebay commented Jan 16, 2018

jorgebay commented Jan 17, 2018

jbmusso commented Jan 17, 2018

jorgebay commented Jan 18, 2018

spmallette commented Jan 18, 2018

jorgebay commented Jan 19, 2018

jbmusso commented Jan 19, 2018

jorgebay commented Jan 22, 2018

jorgebay commented Jan 23, 2018

jbmusso commented Jan 23, 2018

spmallette commented Jan 24, 2018

TINKERPOP-1489 JavaScript GLV #695

TINKERPOP-1489 JavaScript GLV #695

Conversation

jorgebay commented Aug 17, 2017

jorgebay commented Aug 29, 2017

spmallette commented Sep 1, 2017

spmallette commented Sep 5, 2017

jorgebay commented Sep 6, 2017

spmallette commented Sep 7, 2017

jorgebay commented Sep 8, 2017

spmallette commented Sep 8, 2017

spmallette commented Sep 25, 2017

jorgebay commented Nov 30, 2017

spmallette commented Nov 30, 2017

jorgebay commented Jan 4, 2018

spmallette Jan 4, 2018

Choose a reason for hiding this comment

jorgebay Jan 8, 2018

Choose a reason for hiding this comment

spmallette Jan 4, 2018

Choose a reason for hiding this comment

spmallette Jan 4, 2018

Choose a reason for hiding this comment

spmallette Jan 4, 2018

Choose a reason for hiding this comment

jorgebay commented Jan 8, 2018

spmallette commented Jan 9, 2018

jorgebay commented Jan 10, 2018

jbmusso commented Jan 11, 2018

jorgebay commented Jan 16, 2018

jorgebay commented Jan 17, 2018

jbmusso commented Jan 17, 2018

jorgebay commented Jan 18, 2018

spmallette commented Jan 18, 2018

jorgebay commented Jan 19, 2018

jbmusso commented Jan 19, 2018

jorgebay commented Jan 22, 2018

jorgebay commented Jan 23, 2018

jbmusso commented Jan 23, 2018

spmallette commented Jan 24, 2018