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 https://github.com/apache/tinkerpop/pull/1070 and https://github.com/apache/tinkerpop/pull/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 :white_check_mark:

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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: 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.

[kmcginnes]

I've reviewed through the code and it looks great overall. Nice work @tien.

I called out a few areas of potential concern that could use some attention.

It would also be nice to have some automated tests that execute within the context of a browser to ensure we don't regress in the future. Perhaps Playwright can help with this. This is likely to be a large task, so we might want to do it as a separate set of work.

[tien]

@kmcginnes Thanks for taking a look at this, regarding testing in a browser env, yes that would potentially be a much bigger change than this PR 😅, so I think it's best done as a separate task.

[kenhuuu]

You are targetting this PR against the master branch which is currently for the next major release (4.0). The way our branching system works is that you target the lowest version that you want the change to go into. Then when your PR gets merged, the committer will merge your changes all the way up to the latest master branch. It's a bit difficult to explain so I'll use an example. If you want your changes to go into the next 3.6.x, 3.7.x and 4.x release, then you would target the 3.6-dev branch and it will get merged up for you. If you target master then it will only go into the next 4.x release.

I say this because I believe that currently there is no timeline for releasing 4.0 as we are doing releases for 3.6.x and 3.7.x. So if you were to target master then you may need to wait for a while for this change to get released. However, because its currently targetting an unreleased branch, you are technically allowed to upgrade the Node version and ES version.

So, do you prefer to wait for this change to arrive in 4.x? In which case, you can make Node and ES version changes. Or, if you want it released sooner then you will probably have to undo your changes to ESLint and target the 3.7-dev branch as some of those changes could be considered breaking.

[tien]

@kenhuuu Node 18 is compatible with all ES2022 features, so the changes to ESLint should be good.

Regarding the Node version, I'm okay with keeping it 18 now. Since it would be beneficial for me if this is available before 4.0.

I'm integrating gremlin with some of the things at my work & it's easier to justify installing a dependency from an official source, rather than my fork :))

UPDATE: I've updated the target branch to 3.7-dev

[vkagamlyk]

Is there way to simply test these changes in different browsers?

@tien maybe you have a simple application that can be put into examples and run to check the basic functionality of the driver? This can also be used as an example for users

[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.