[GitHub] tinkerpop issue #695: TINKERPOP-1489 JavaScript GLV
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/695 All tests pass with `docker/build.sh -t -n -i` VOTE +1 ---
[GitHub] tinkerpop issue #695: TINKERPOP-1489 JavaScript GLV
Github user jbmusso commented on the issue: https://github.com/apache/tinkerpop/pull/695 All good to me! VOTE +1 ---
[GitHub] tinkerpop issue #695: TINKERPOP-1489 JavaScript GLV
Github user jorgebay commented on the issue: https://github.com/apache/tinkerpop/pull/695 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. ---
[GitHub] tinkerpop issue #695: TINKERPOP-1489 JavaScript GLV
Github user jorgebay commented on the issue: https://github.com/apache/tinkerpop/pull/695 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](https://github.com/nodejs/Release) and it didn't support several ES2015 features (rest parameters being one of them). ---
[GitHub] tinkerpop issue #695: TINKERPOP-1489 JavaScript GLV
Github user jbmusso commented on the issue:
https://github.com/apache/tinkerpop/pull/695
According to https://github.com/petkaantonov/bluebird/issues/1026, users
should be able to just patch the global `Promise` object in their application
with:
```javascript
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).
---
[GitHub] tinkerpop issue #695: TINKERPOP-1489 JavaScript GLV
Github user jorgebay commented on the issue: https://github.com/apache/tinkerpop/pull/695 Rebased. ---
[GitHub] tinkerpop issue #695: TINKERPOP-1489 JavaScript GLV
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/695 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. ---
[GitHub] tinkerpop issue #695: TINKERPOP-1489 JavaScript GLV
Github user jorgebay commented on the issue:
https://github.com/apache/tinkerpop/pull/695
> 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):
```javascript
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:
```javascript
const statics = {
V: (...args) => callOnEmptyTraversal('V', args),
addE: (...args) => callOnEmptyTraversal('addE', args),
// ...
};
```
wdyt?
I'll start working on the other suggestions (ES6).
---
[GitHub] tinkerpop issue #695: TINKERPOP-1489 JavaScript GLV
Github user jbmusso commented on the issue: https://github.com/apache/tinkerpop/pull/695 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](http://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](https://www.npmjs.com/package/babel-preset-env) combined with [postinstall-build](https://www.npmjs.com/package/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. ---
[GitHub] tinkerpop issue #695: TINKERPOP-1489 JavaScript GLV
Github user jorgebay commented on the issue: https://github.com/apache/tinkerpop/pull/695 Thanks @dkuppitz for looking into the maven issue! I'll rebase it and add a `g:T` deserializer. ---
[GitHub] tinkerpop issue #695: TINKERPOP-1489 JavaScript GLV
Github user jorgebay commented on the issue: https://github.com/apache/tinkerpop/pull/695 I've just noticed that there isn't a `g:T` deserializer, I'll add it in the next days. ---
[GitHub] tinkerpop issue #695: TINKERPOP-1489 JavaScript GLV
Github user jbmusso commented on the issue: https://github.com/apache/tinkerpop/pull/695 Quick update - I plan to check this PR this weekend. ---
[GitHub] tinkerpop issue #695: TINKERPOP-1489 JavaScript GLV
Github user jorgebay commented on the issue: https://github.com/apache/tinkerpop/pull/695 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`. ---
[GitHub] tinkerpop issue #695: TINKERPOP-1489 JavaScript GLV
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/695 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 ? ---
[GitHub] tinkerpop issue #695: TINKERPOP-1489 JavaScript GLV
Github user jorgebay commented on the issue: https://github.com/apache/tinkerpop/pull/695 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. ---
[GitHub] tinkerpop issue #695: TINKERPOP-1489 JavaScript GLV
Github user jorgebay commented on the issue: https://github.com/apache/tinkerpop/pull/695 What a great time of the year to review this pr! ð ---
[GitHub] tinkerpop issue #695: TINKERPOP-1489 JavaScript GLV
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/695 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. ---
[GitHub] tinkerpop issue #695: TINKERPOP-1489 JavaScript GLV
Github user jorgebay commented on the issue: https://github.com/apache/tinkerpop/pull/695 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!)... ---
[GitHub] tinkerpop issue #695: TINKERPOP-1489 JavaScript GLV
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/695 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. ---
[GitHub] tinkerpop issue #695: TINKERPOP-1489 JavaScript GLV
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/695 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. ---
[GitHub] tinkerpop issue #695: TINKERPOP-1489 JavaScript GLV
Github user jorgebay commented on the issue: https://github.com/apache/tinkerpop/pull/695 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. ---
[GitHub] tinkerpop issue #695: TINKERPOP-1489 JavaScript GLV
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/695 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. ---
[GitHub] tinkerpop issue #695: TINKERPOP-1489 JavaScript GLV
Github user jorgebay commented on the issue: https://github.com/apache/tinkerpop/pull/695 Great! It would be really nice to have a working `GremlinJavaScriptScriptEngine`! ---
[GitHub] tinkerpop issue #695: TINKERPOP-1489 JavaScript GLV
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/695 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. ---
[GitHub] tinkerpop issue #695: TINKERPOP-1489 JavaScript GLV
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/695 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. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #695: TINKERPOP-1489 JavaScript GLV
Github user jorgebay commented on the issue:
https://github.com/apache/tinkerpop/pull/695
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:
```js
const vertices = await g.V().toList();
```
```js
for await (const vertex of g.V()) {
console.log(vertex.label);
}
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
