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.

Sign up for GitHub

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

Jump to bottom

feat: Promise-like API for PeerJS #1127

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from
+442 −132
Draft

feat: Promise-like API for PeerJS #1127

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
b2016a5
feat: emit error on connections when receiving "EXPIRE"
jonasgloning Sep 2, 2023
7e18b44
test(e2e): "peer-unavailable"
jonasgloning Aug 26, 2023
0319432
docs: urge the user to listen to the `error` event
jonasgloning Sep 2, 2023
2cba46d
feat: `await new Peer()`
jonasgloning Aug 28, 2023
bee2bdb
feat: `await peer.connect(...)`
jonasgloning Sep 3, 2023
e40007a
some `IPeer` types
jonasgloning Sep 3, 2023
54e2ac6
refactor: `implement Promise` for all PeerJS objects
jonasgloning Sep 5, 2023
15311d9
refactor: change `removeListener` to `off` for consitency
jonasgloning Sep 7, 2023
b49ef4a
refactor: fix event types for library consumers
jonasgloning Sep 7, 2023
5edbfd5
refactor: try to make `id-taken.await` clearer
jonasgloning Sep 7, 2023
b26a8e7
npm run check
jonasgloning Sep 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
48 changes: 48 additions & 0 deletions e2e/peer/id-taken.await.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
<!doctype html>
<html lang="en">
<head>
<meta charset="UTF-8" />
<meta name="viewport" content="width=device-width, initial-scale=1.0" />
<title></title>
<link rel="stylesheet" href="../style.css" />
</head>
<body>
<h1>ID-TAKEN</h1>
<div id="messages"></div>
<div id="error-message"></div>
<script src="/dist/peerjs.js"></script>
<script type="application/javascript">
(async () => {
/**
* @type {typeof import("../../lib/exports.ts").Peer}
*/
const Peer = window.peerjs.Peer;

const messages = document.getElementById("messages");
const errorMessage = document.getElementById("error-message");

// Peer A should be created without an error
try {
const peerA = await new Peer();
// Create 10 new `Peer`s that will try to steel A's id
let peers_try_to_take = Array.from({ length: 10 }, async (_, i) => {
try {
await new Peer(peerA.id);
throw `Peer ${i} failed! Connection got established.`;
} catch (error) {
if (error.type === "unavailable-id") {
return `ID already taken. (${i})`;
} else {
throw error;
}
}
});
await Promise.all(peers_try_to_take);
messages.textContent = "No ID takeover";
} catch (error) {
errorMessage.textContent += JSON.stringify(error);
}
})();
</script>
</body>
</html>
41 changes: 41 additions & 0 deletions e2e/peer/peer-unavailable.async.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
<!doctype html>
<html lang="en">
<head>
<meta charset="UTF-8" />
<meta name="viewport" content="width=device-width, initial-scale=1.0" />
<title></title>
<link rel="stylesheet" href="../style.css" />
</head>
<body>
<h1>PEER-UNAVAILABLE</h1>
<div id="messages"></div>
<div id="error-message"></div>
<script src="/dist/peerjs.js"></script>
<script type="application/javascript">
(async () => {
/**
* @type {typeof import("../../lib/exports.ts").Peer}
*/
const Peer = window.peerjs.Peer;

const messages = document.getElementById("messages");
const errors = document.getElementById("error-message");

const not_existing_peer = crypto
.getRandomValues(new Uint8Array(16))
.join("");

try {
const peer = await new Peer();
await peer.connect(not_existing_peer);
} catch (error) {
if (error.type === "peer-unavailable") {
messages.textContent = "Success: Peer unavailable";
} else {
errors.textContent += JSON.stringify(error);
}
}
})();
</script>
</body>
</html>
43 changes: 43 additions & 0 deletions e2e/peer/peer-unavailable.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
<!doctype html>
<html lang="en">
<head>
<meta charset="UTF-8" />
<meta name="viewport" content="width=device-width, initial-scale=1.0" />
<title></title>
<link rel="stylesheet" href="../style.css" />
</head>
<body>
<h1>PEER-UNAVAILABLE</h1>
<div id="messages"></div>
<div id="error-message"></div>
<script src="/dist/peerjs.js"></script>
<script type="application/javascript">
/**
* @type {typeof import("../..").Peer}
*/
const Peer = window.peerjs.Peer;

const connectionErrors = document.getElementById("messages");
const peerErrors = document.getElementById("error-message");

const not_existing_peer = crypto
.getRandomValues(new Uint8Array(16))
.join("");

const peer = new Peer();
peer
.once(
"error",
(error) => void (peerErrors.textContent += JSON.stringify(error)),
)
.once("open", (id) => {
const connection = peer.connect(not_existing_peer);
connection.once(
"error",
(error) =>
void (connectionErrors.textContent += JSON.stringify(error)),
);
});
</script>
</body>
</html>
17 changes: 17 additions & 0 deletions e2e/peer/peer.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,21 @@ describe("Peer", () => {
await P.waitForMessage('{"type":"disconnected"}');
expect(await P.errorMessage.getText()).toBe("");
});
it("should emit an error, when the remote peer is unavailable", async () => {
await P.open("peer-unavailable");
await P.waitForMessage('{"type":"peer-unavailable"}');
expect(await P.errorMessage.getText()).toBe('{"type":"peer-unavailable"}');
});
});
describe("Peer:async", () => {
it("should emit an error, when the ID is already taken", async () => {
await P.open("id-taken.await");
await P.waitForMessage("No ID takeover");
expect(await P.errorMessage.getText()).toBe("");
});
it("should emit an error, when the remote peer is unavailable", async () => {
await P.open("peer-unavailable.async");
await P.waitForMessage("Success: Peer unavailable");
expect(await P.errorMessage.getText()).toBe("");
});
});
29 changes: 27 additions & 2 deletions lib/dataconnection/DataConnection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { BaseConnection, type BaseConnectionEvents } from "../baseconnection";
import type { ServerMessage } from "../servermessage";
import type { EventsWithError } from "../peerError";
import { randomToken } from "../utils/randomToken";
import { PeerError } from "../peerError";

export interface DataConnectionEvents
extends EventsWithError<DataConnectionErrorType | BaseConnectionErrorType>,
Expand All @@ -25,6 +26,14 @@ export interface DataConnectionEvents
open: () => void;
}

export interface IDataConnection
extends BaseConnection<DataConnectionEvents, DataConnectionErrorType> {
/** Allows user to close connection. */
close(options?: { flush?: boolean }): void;
/** Allows user to send data. */
send(data: any, chunked?: boolean): void;
}

/**
* Wraps a DataChannel between two Peers.
*/
Expand All @@ -38,6 +47,10 @@ export abstract class DataConnection extends BaseConnection<
private _negotiator: Negotiator<DataConnectionEvents, this>;
abstract readonly serialization: string;
readonly reliable: boolean;
private then: (
onfulfilled?: (value: IDataConnection) => any,
onrejected?: (reason: PeerError<DataConnectionErrorType>) => any,
) => void;

public get type() {
return ConnectionType.Data;
Expand All @@ -46,6 +59,20 @@ export abstract class DataConnection extends BaseConnection<
constructor(peerId: string, provider: Peer, options: any) {
super(peerId, provider, options);

this.then = (
onfulfilled?: (value: IDataConnection) => any,
onrejected?: (reason: PeerError<DataConnectionErrorType>) => any,
) => {
// Remove 'then' to prevent potential recursion issues
// `await` will wait for a Promise-like to resolve recursively
delete this.then;
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this mean you cannot do e.g.

const connectionPromise = peer.connect(peer);
connectionPromise.then(() => console.log(1));
connectionPromise.then(() => console.log(2));

and

const connectionPromise = peer.connect(peer);
connectionPromise
  .then(() => console.log(1))
  .then(() => console.log(2))
  .catch(e => console.error(e))

? Also I think this will behave in an unexpected way, because the second await would finish before the connection is actually open:

const connectionPromise = peer.connect(peer);
(async () => {
  await connectionPromise;
  console.log('connected');
})();
(async () => {
  await connectionPromise;
  console.log('connected');
  console.error('Sike! Not connected');
})();

While I think the first two examples are fine as long as it's clear to the users that it's not a perfect Promise implementation, the third one seems dangerous to me, it would be very hard to catch such a bug.

Here's a doc I found on how to make a good thenable: https://promisesaplus.com/#point-36

I think there must be an easy way to implement this by proxying then (and catch?) to a real Promise object.

Copy link
Member Author

Choose a reason for hiding this comment

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

The original intention was to use then as a hack to be able to use await. It was even marked private as to explicitly disallow the first two.
But great suggestion, there's no good reason for this. All PeerJS objects now implement Promise (including catch/finally). then internally constructs and returns a real Promise, meaning the first two work correctly.

You are totally right, 3 is confusing. I hope I adequately addressed this with this

peerjs/lib/eventEmitterWithPromise.ts

Lines 43 to 45 in 15311d9

// Remove 'then' to prevent potential recursion issues
// `await` will wait for a Promise-like to resolve recursively
resolve?.(proxyWithoutThen(this));

Instead of patching this at runtime I create a Proxy without a then property. Subsequent calls to then will have the some consistent behavior.


// We don’t need to worry about cleaning up listeners here
// `await`ing a Promise will make sure only one of the paths executes
this.once("open", () => onfulfilled(this));
this.once("error", onrejected);
Copy link
Contributor

@WofWca WofWca Sep 4, 2023 •
edited

Choose a reason for hiding this comment

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

Though I believe the listeners would still remain in memory (a.k.a. a memory leak). Though currently then can only be executed once, so no big deal.
If not, some apps might have async code that does something like connectionPromise.then(c = c.send('something')).

Copy link
Member Author

Choose a reason for hiding this comment

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

Great point. Now removing the other listener when firing an event:

peerjs/lib/eventEmitterWithPromise.ts

Lines 41 to 50 in 15311d9

const onOpen = () => {
this.off("error", onError);
// Remove 'then' to prevent potential recursion issues
// `await` will wait for a Promise-like to resolve recursively
resolve?.(proxyWithoutThen(this));
};
const onError = (err: PeerError<`${ErrorType}`>) => {
this.off("open", onOpen);
reject(err);
};

};

this.connectionId =
this.options.connectionId || DataConnection.ID_PREFIX + randomToken();

Expand Down Expand Up @@ -87,7 +114,6 @@ export abstract class DataConnection extends BaseConnection<
* Exposed functionality for users.
*/

/** Allows user to close connection. */
close(options?: { flush?: boolean }): void {
if (options?.flush) {
this.send({
Expand Down Expand Up @@ -126,7 +152,6 @@ export abstract class DataConnection extends BaseConnection<

protected abstract _send(data: any, chunked: boolean): void;

/** Allows user to send data. */
public send(data: any, chunked = false) {
if (!this.open) {
this.emitError(
Expand Down
1 change: 1 addition & 0 deletions lib/enums.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ export enum PeerErrorType {
}

export enum BaseConnectionErrorType {
PeerUnavailable = "peer-unavailable",
NegotiationFailed = "negotiation-failed",
ConnectionClosed = "connection-closed",
}
Expand Down