jbmusso / gremlin-javascript

JavaScript tools for graph processing in Node.js and the browser inspired by the Apache TinkerPop API
MIT License
214 stars 62 forks source link

Update ws dependency to 1.x #74

Closed princjef closed 7 years ago

princjef commented 7 years ago

Bumps the ws dependency up to 1.x, resolving issues like those seen in #60.

Also fixes the execute() method to work with the updated version of babel, which doesn't generate the same code for methods that combine optional and rest parameters as previous versions (a.k.a. what built version 2.3.2 of this library)

coveralls commented 7 years ago

Coverage Status

Changes Unknown when pulling 8561d3541bdcf2bb2efc8332c841f11317c32cfb on princjef:ws-update into on jbmusso:master.

jbmusso commented 7 years ago

Thanks for the PR! I'm still unsure about the switch back from ...argsto arguments. IIRC, arguments is deprecated in favor of ...args. Maybe we need to investigate and fix the Babel build instead.

princjef commented 7 years ago

afaik the rest operator and arguments have separate meanings and per MDN, arguments hasn't been deprecated.

As I was debugging the test issue, I looked through the generated output of both the published code (2.3.2) and the output when compiling against latest in master with a fresh npm install. I snagged screenshots so you can see the behavior of both:

Published (2.3.2) image

Master image

The callback line is the only one that changed in a substantive way. From what I can tell, the version that's published had a transpiler bug, since the two 3s in the arguments computation cancel out. With that behavior, even if you only provide one argument to the function, it will be assigned to the callback variable. I'm not sure how optional/default params and rest params combine, but ...args definitely shouldn't be overlapping with script.

I looked around a bit in the babel issues and found this. I'm not sure if it stems from the same bug, but it's a similar problem (treating a trailing rest param as if it represents the entire argument list).

guyellis commented 7 years ago

@jbmusso this looks like a good PR to me.

Accessing the callback at the end of the arguments using the ES5 pattern instead of the ES6 pattern like @princjef has done is safer as it's not relying on the compilation and nuances of Babel.

guyellis commented 7 years ago

In order for me to move ahead with this PR in my work I've created a master-alt branch in my fork and I've published it on npmjs as gremlin-alt. Version 1.0.1 of my publish has the changes from this PR.

My production app that uses this module has a number of integration tests that all pass with this alternate publish.

This gremlin-alt module is just a temporary solution and I plan to continue to use this module once you've had the chance to publish this PR and some others.

jbmusso commented 7 years ago

Thanks, I'll review it this weekend and most likely merge/publish :). Unfortunately I lack free time at the moment.

jbmusso commented 7 years ago

That didn't take that long actually. Publishing new release on npm in a few minutes.

princjef commented 7 years ago

thanks @jbmusso and @guyellis!