Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TINKERPOP-2143 JavaScript GLV: Support browsers #1291

Closed
wants to merge 1 commit into from

Conversation

spmallette
Copy link
Contributor

https://issues.apache.org/jira/browse/TINKERPOP-2143

The original PR for this is at #1070. The changes were simply brought here for ongoing rebasing until someone picks up the work and completes it. As it stands there are failing tests on a standard build.

@jorgebay
Copy link
Contributor

jorgebay commented Jun 2, 2020

I'll review it asap.

@jorgebay
Copy link
Contributor

jorgebay commented Jun 9, 2020

cc @fdominik

Copy link
Contributor

@jorgebay jorgebay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It still has a long way to go before it can be merged. I highlighted a few issues below.

@@ -23,7 +23,8 @@
'use strict';

const EventEmitter = require('events');
const WebSocket = require('ws');
//const WebSocket = require('ws');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This import should still be included.

const message = Buffer.from(this._header + JSON.stringify(this._getRequest(requestId, bytecode, op, args, processor)));
this._ws.send(message);
}));
//const message = Buffer.from(this._header + JSON.stringify(this._getRequest(requestId, bytecode, op, args, processor)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leftover.

const message = this._header + JSON.stringify(this._getRequest(requestId, bytecode, op, args, processor));
var buf = new ArrayBuffer(message.length); // 2 bytes for each char
var bufView = new Uint8Array(buf);
for (var i=0, strLen=message.length; i < strLen; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably this needs a TextEncoder or something like that for fast access: https://developer.mozilla.org/en-US/docs/Web/API/TextEncoder

There are buffer libraries that use built-in Buffer in Node.js and polyfills for browsers, we should use one of those.

Comment on lines 142 to 147
this._ws.onopen = (() => {
this.isOpen = true;
if (this._pingEnabled) {
this._pingHeartbeat();
}
resolve();
});
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation is wrong.

* Change the way how random bytes are generated, not using the crypto package, but built-in Class.

update dependency on ws package to version 6+...
* remove requiring the ws package
* update how WebSockets are called.

fix to use better library for creating UUID v4...

better use of standard Buffer of Browser instead of Buffer class from NodeJS (not available in Browsers)

Don't require crypto package anymore as it is built-in by default.
* Change the way how random bytes are generated, not using the crypto package, but built-in Class.

update dependency on ws package to version 6+...
* remove requiring the ws package
* update how WebSockets are called.

(cherry picked from commit b016cd1)

update dependency on ws package to version 6+...
* remove requiring the ws package
* update how WebSockets are called.

Update README.asciidoc

update WS handling so that correct response is parsed.

fix for wrong check in handling Message

Revert "Update README.asciidoc"

This reverts commit 0ba06a3 as it was unintentioanlly comitted to where PR will be created

clean the console.log used for debugging only

trying to fix the failing Travis build
@jorupp
Copy link

jorupp commented Feb 4, 2022

Something else to think about here - some of the new edge-compute stuff (like Cloudflare workers) run a Browser-like JS environment as well, so supporting browsers should indirectly support environments like that.

At least I think so - I get an error "ws does not work in the browser. Browser clients must use the native WebSocket object" when using the gremlin JS SDK in a cloudflare worker, so I'd hope that working in a browser would get it working there too.

@spmallette
Copy link
Contributor Author

closing this PR for now since it's now heavily conflicted and had remaining items to be addressed. if there is interested in reviving this work, please feel free to open a fresh PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants