[GitHub] tinkerpop issue #695: TINKERPOP-1489 JavaScript GLV

2018-01-24 Thread spmallette
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

2018-01-23 Thread jbmusso
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

2018-01-23 Thread jorgebay
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

2018-01-22 Thread jorgebay
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

2018-01-19 Thread jbmusso
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

2018-01-19 Thread jorgebay
Github user jorgebay commented on the issue:

https://github.com/apache/tinkerpop/pull/695
  
Rebased.


---


[GitHub] tinkerpop issue #695: TINKERPOP-1489 JavaScript GLV

2018-01-18 Thread spmallette
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

2018-01-18 Thread jorgebay
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

2018-01-17 Thread jbmusso
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

2018-01-17 Thread jorgebay
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

2018-01-16 Thread jorgebay
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

2018-01-11 Thread jbmusso
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

2018-01-10 Thread jorgebay
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

2018-01-09 Thread spmallette
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

2018-01-08 Thread jorgebay
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

2018-01-04 Thread jorgebay
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

2017-11-30 Thread spmallette
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

2017-11-30 Thread jorgebay
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

2017-09-25 Thread spmallette
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

2017-09-08 Thread spmallette
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

2017-09-08 Thread jorgebay
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

2017-09-07 Thread spmallette
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

2017-09-06 Thread jorgebay
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

2017-09-05 Thread spmallette
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

2017-09-01 Thread spmallette
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

2017-08-29 Thread jorgebay
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.
---