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

DEV-269 Use isomorphic-ws instead of ws #443

Merged
merged 3 commits into from
Mar 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions lib/graphql/client.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import * as ws from "ws";
import { WebSocket } from "isomorphic-ws";

import { Client, createClient } from "graphql-ws";
import { GraphQLError, UserUnauthorizedError } from "../errors";
Expand Down Expand Up @@ -131,12 +131,11 @@ export class GraphQLClient {
private createSubscriptionClient = (): Client => {
const auth = this.getAuthHeader();
const connectionParams = auth ? { [auth[0]]: auth[1] } : {};
const webSocketImpl = typeof window === "undefined" ? ws : window.WebSocket;

return this.createClient({
url: this.subscriptionEndpoint,
connectionParams,
webSocketImpl,
webSocketImpl: WebSocket,
})
}

Expand Down
101 changes: 64 additions & 37 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,10 @@
"graphql": "^16.3.0",
"graphql-request": "^4.0.0",
"graphql-ws": "^5.5.5",
"isomorphic-ws": "^5.0.0",
"js-sha256": "^0.9.0",
"querystring-es3": "^0.2.1",
"ws": "^8.4.2"
"ws": "^8.16.0"
Comment on lines -28 to +29
Copy link
Contributor

Choose a reason for hiding this comment

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

❔ Why did you downgrade ws library?
Also could it be totally replaced by isomorphic-ws library?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ws is actually upgraded from 4 to 16 😉 It's just unusual to read when version number has 2 digits

isomorphic-ws internally uses ws for NodeJS and they require to have ws package installed as "peer dependency".

Copy link
Contributor

@Campalo Campalo Mar 4, 2024

Choose a reason for hiding this comment

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

ws is actually upgraded from 4 to 16 😉 It's just unusual to read when version number has 2 digits

🙈

},
"overrides": {
"tough-cookie": "4.1.3"
Expand Down
33 changes: 5 additions & 28 deletions tests/graphql/client.spec.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import * as sinon from "sinon";
import * as ws from "ws";
import { WebSocket } from "isomorphic-ws";

import { GraphQLError, UserUnauthorizedError } from "../../lib/errors";

Expand Down Expand Up @@ -540,39 +540,16 @@ describe("createSubscriptionClient", () => {
it("should create a new SubscriptionClient and return it", () => {
const subscriptionClient = client.graphQL.createSubscriptionClient();
expect(createClientStub.callCount).to.equal(1);
const {url, connectionParams} = createClientStub.getCall(
0,
).args[0];
const { url, connectionParams, webSocketImpl } =
createClientStub.getCall(0).args[0];

expect(url).to.equal(
`${KONTIST_SUBSCRIPTION_API_BASE_URL}/api/graphql`,
);
expect(url).to.equal(`${KONTIST_SUBSCRIPTION_API_BASE_URL}/api/graphql`);
expect(connectionParams).to.deep.equal({
Authorization: "Bearer dummy-token",
});
expect(webSocketImpl).to.equal(WebSocket);
expect(subscriptionClient).to.equal(fakeGraphqlWsClient);
});

describe("when executing in a browser environment", () => {
before(() => {
(global as any).window = {
WebSocket: {...ws, fake: true},
};
});

after(() => {
(global as any).window = undefined;
});

it("should use the native browser WebSocket implementation", () => {
createClientStub.resetHistory();

client.graphQL.createSubscriptionClient();

expect(createClientStub.callCount).to.equal(1);
expect(createClientStub.getCall(0).args[0].webSocketImpl.fake).to.equal(true);
});
});
Comment on lines -555 to -575
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 why we are removing test? we can still verify if isomorphic-ws using native WebSocket with this

Copy link
Contributor Author

@dmytroyarmak dmytroyarmak Mar 4, 2024

Choose a reason for hiding this comment

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

This test case was specifically for typeof window === "undefined" condition that was in our code base, but now it's removed and it's now internal logic of isomorphic-ws. They actually handle it in more graceful way by having browser entry point in package.json (webpack uses it during bundling) that loads very tiny JS wrapper to normalize native websocket and for node.js it loads completely different file with ws implementation.

Anyway, I've extendede previous test case to verify that we pass WebSocket from isomorphic-ws to createClient: d5600cc

});
});

Expand Down
Loading