feat: gremlin-javascript browser support

[tien]

• Replace node APIs that have cross runtime equivalent
• For those that can't be replaced, use node: prefix so that environment that does polyfill automatically can easily do so
• Remove manual ping/pong logic, this is already handled internally by browser or ws library + WhatWG WebSocket API doesn't have ability to do manual ping/pong

[tien]

Hey guys, would love to get this working in an environment outside of Node.js.

I think this would boost adoption greatly, as a lot of serverless runtimes (i.e. Cloudflare Worker, Vercel, etc) now utilise standard Web APIs.

This is just something I draft quickly together with the limited knowledge that I have. Would appreciate any helps & guidances. 🙏

[kenhuuu]

Thanks for you interest in contributing. There has been a lot of interest around adding this support in the past. This is the open Jira issue for reference https://issues.apache.org/jira/browse/TINKERPOP-2143. The past attempts seem to have run into some issues with tests #1070 and #1291. I'm not familiar with JavaScript so I'll try to have someone that is look into this.

Thanks again.

[tien]

@kenhuuu thanks for getting back & reminding me about the integration test, I was able to run it locally and have all tests passed. For actually running in a browser API compatible environment, I've tested this on my application running on Cloudflare Worker & everything so far works as expected.

[tien]

Have also just tested & fix bugs when running this in an actual browser. Everything seems okay so far.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.49%. Comparing base (9b46b67) to head (7c1d5ec).
Report is 35 commits behind head on 3.7-dev.

Additional details and impacted files

@@              Coverage Diff              @@
##             3.7-dev    #2506      +/-   ##
=============================================
+ Coverage      76.14%   76.49%   +0.35%     
- Complexity     13152    13170      +18     
=============================================
  Files           1084     1059      -25     
  Lines          65160    61270    -3890     
  Branches        7285     7295      +10     
=============================================
- Hits           49616    46869    -2747     
+ Misses         12839    11888     -951     
+ Partials        2705     2513     -192     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tien]

Forgot that globalThis.crypto is not available on Node 18, only 19+ (hence the failing test on CI), so I have made a change to use uuid instead.

But optionally we can change the supported node version to LTS, which is 20 right now if you guys think that is best.

[tien]

Hmm, looks like the .nvmrc file was picked up by the license checker. .nvmrc files can't contain comment, so I have added it to the list of ignored files.

[kenhuuu]

But optionally we can change the supported node version to LTS, which is 20 right now if you guys think that is best.

It's against our current runtime policy (https://tinkerpop.apache.org/docs/current/dev/developer/#runtimes) to upgrade unless the supported runtime has reached end of life. If you feel that it would really be worth it to upgrade to Node 20 then it's a conversation you could start on the dev-list.

Hmm, looks like the .nvmrc file was picked up by the license checker. .nvmrc files can't contain comment, so I have added it to the list of ignored files.

I think that makes sense to ignore it then.

[tien]

@kenhuuu Regarding the Node version, whatever make sense to you guys, of course. It's not a major deal.

But I think the wording in the link that you shared could be a bit better:

Each programming language has a runtime that TinkerPop supports. In general, TinkerPop will attempt to support the current LTS version for a particular major version for the lifetime of its minor and patch releases.

It's not clear whether that mean supporting the current LTS version, which is 20. Or supporting previous version that hasn't reached end of life yet, which is 18.

[tien]

@vkagamlyk the proper way would be to actually run the unit tests in the browser, like @kmcginnes mentioned.

But that could potentially be a huge change to the current test suite.

Another way would be to inject a browser WebSocket polyfill into the current test suite using jsdom. It's not perfect but would require a lot less changes.

[tien]

@vkagamlyk also, because the API signature is the exact same, those existing examples can be run just fine in a browser environment.

[tien]

@vkagamlyk or are you suggesting I should create like a simple html page that simply run all the stuffs under examples?

[vkagamlyk]

@vkagamlyk or are you suggesting I should create like a simple html page that simply run all the stuffs under examples?

I thought about something more simple, like html page which runs single query like g.V().group().by(label).by(count()) and show results.
This will be useful for those who want to use Gremlin from the browser, and it will allow developers to relatively easily check the functionality of the driver after changes.

[tien]

Okay I'll add a simple webpage. There's some flaky test that is failing randomly on GitHub Action it seems btw.

[tien]

@vkagamlyk have added a simple webpage that just display your graph running on localhost on the screen.

[tien]

Modified the README a bit to say this is also compatible with Web APIs compatible runtimes (browsers, WinterCG runtimes, etc)

[vkagamlyk]

@tien I got an error index.js:22 Uncaught SyntaxError: The requested module '/@fs/C:/_Projects/tinkerpop-tien/gremlin-javascript/src/main/javascript/gremlin-javascript/index.js' does not provide an export named 'default' (at index.js:22:8) in Chrome.

[tien]

@tien I got an error index.js:22 Uncaught SyntaxError: The requested module '/@fs/C:/_Projects/tinkerpop-tien/gremlin-javascript/src/main/javascript/gremlin-javascript/index.js' does not provide an export named 'default' (at index.js:22:8) in Chrome.

Is this from running the example? To run the example, you need to do this at tinkerpop/gremlin-javascript/examples/browser:

yarn
yarn dev

[vkagamlyk]

@tien I got an error index.js:22 Uncaught SyntaxError: The requested module '/@fs/C:/_Projects/tinkerpop-tien/gremlin-javascript/src/main/javascript/gremlin-javascript/index.js' does not provide an export named 'default' (at index.js:22:8) in Chrome.

Is this from running the example? To run the example, you need to do this at tinkerpop/gremlin-javascript/examples/browser:

yarn
yarn dev

ahh, I tried with npm.
Looks nice

[vkagamlyk]

VOTE +1

[tien]

VOTE +1

Thanks 🙏, just made a small change, updating the browser example package.json description.

[tien]

@kenhuuu regarding the user agent header used by Gremlin server

This is likely a separate discussion, but User-Agent header might not be the best way to identify client supported GLV version. Since browser environment for example doesn't allow you to set or modify this for web socket connection.

What might be better is using the Sec-WebSocket-Protocol header instead. Which is designed to specify the sub-protocol requested by client & can be set in the browser by doing:

new WebSocket("ws://localhost:8182", "glv3.7");

The sub-protocol can even be registered here if needed :))

[kenhuuu]

@kenhuuu regarding the user agent header used by Gremlin server

This is likely a separate discussion, but User-Agent header might not be the best way to identify client supported GLV version. Since browser environment for example doesn't allow you to set or modify this for web socket connection.

What might be better is using the Sec-WebSocket-Protocol header instead. Which is designed to specify the sub-protocol requested by client & can be set in the browser by doing:

new WebSocket("ws://localhost:8182", "glv3.7");

The sub-protocol can even be registered here if needed :))

Thanks for the information. As I mentioned above, we'll be moving to HTTP only in 4.x so its probably best to keep the 3.x WebSocket behavior the same for now.

[kenhuuu]

VOTE +1. Thanks again for your willingness to contribute. It is much appreciated.

I'm not sure if you are familiar with our policy but it takes a VOTE+1 from three different committers to allow a PR to get merged immediately. Otherwise, if you get at least one of them (you have two right now), then the PR can be merged one week after the first VOTE +1. So at the latest this PR will be eligible for merging on Monday of next week.

[tien]

@kenhuuu thanks for this. I'll try to find another VOTE +1 if possible. Would be beneficial for me to get this in early because there's a work project I'm on that's currently using this.

[Cole-Greer]

Thanks for putting all of this together @tien, that's awesome that gremlin-javascript will now work in browsers and I think having that simple little visualizer as an example is really cool. I do have a slight apprehension towards advertising full support for browser runtimes without a proper test suite for it. How would you feel about changing the wording in the readme to something such as Gremlin-Javascript implements Gremlin within the JavaScript language and can be used on Node.js and has experimental support for Web APIs compatible runtimes. I would be more comfortable listing it as full support after it has gone through a few release cycles and we have more confidence that browser compatibility is stable and we aren't likely to accidentally break it with the absence of tests.

[tien]

@Cole-Greer Yeah I think that make sense, have changed the README accordingly.

[Cole-Greer]

Thanks @tien, I think this is great. VOTE +1. I will merge this into both 3.7-dev and master so it will be reflected in the upcoming 3.7.2 and 4.0.0 releases. Just to set some expectations on release timelines, no discussion has yet taken place on when the 3.7.2 release will take place, but based on historical release timing, we typically see a release in the rough April-May window. I would expect that discussions around the next release will begin to take place in the dev maillist sometime within the next couple of weeks.

[Cole-Greer]

Just to set some expectations on release timelines, no discussion has yet taken place on when the 3.7.2 release will take place, but based on historical release timing, we typically see a release in the rough April-May window. I would expect that discussions around the next release will begin to take place in the dev maillist sometime within the next couple of weeks.

I hadn't checked my email just yet when I wrote this, looks like the release discussion has just started. The current proposal is for a early April release. https://lists.apache.org/thread/8vvo6f74lo1tb7pzclo1pwqbv6xrncl3