feat: Promise-like API for PeerJS

[jonasgloning]

Hey @WofWca, you recently proposed a Promise-based API in #924 (comment).

This is a quick draft of how this could look like. Is this roughly how you would imagine it?

(Only for Peer, connections are not included yet).

Implementation

Beta Give feedback

1. private then: (onfulfilled?: (value: IDataConnection) => any, onrejected?: (reason: PeerError<DataConnectionErrorType>) => any,) => void;
2. Mediastream

Documentation

Beta Give feedback

1. Inline
2. Website

[jonasgloning]

Hey @WofWca, I just added add await peer.connect(...):

const peer = new Peer();
peer.once("open", id => {
  const conn = peer.connect(remote_id)
  conn.once("open, () => {
   /* connection established */
  })
})

should be equivalent to

const peer = await new Peer();
const conn = await peer.connect(remote_id);
/* connection established */

Example with error handling:

peerjs/e2e/peer/peer-unavailable.async.html

Lines 28 to 38 in bee2bdb

	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);
	}
	}
	})();

---

I'd love a quick review, if you have the time, to see if this meets your expectations.

[WofWca]

Hey, sorry for such a late response.
I stated my concern about a thing. Otherwise overall looks like an improvement. I hope it doesn't become a maintenance burden.

Making DataConnection thenable is a fun and original idea, though I was thinking about making connect() return an actual Promise (or, for a non-breaking change, adding a new function that does this), since it's not really possible to send() on a DataConnection that is not open, and send() is the main purpose of DataConnection.
I'm not sure if it's a good idea though. It sounds like a trade-off between control and convenience. Maybe having two functions - one Promise-based, the other like it is now - is the way.

Also if we're going for this I think it makes sense to do the same for MediaConnection.

[WofWca]

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')).

[jonasgloning]

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);
	};

[WofWca]

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.

[jonasgloning]

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.

[jonasgloning]

Thank you for your feedback! Please let me know if there's anything I can do to incentivize you to review future pull-requests.

I favor making Peer and DataConnection thenable on their own instead of creating a new method that returns a Promise, as it enables the setting of event listeners in a single step:

const peer = await new Peer().on("call", ...).on("disconnected", ...).once("open", ...)
const conn = await peer.connect().on("data", ...)

[WofWca]

Sorry for such a late response. You asked a tough question.

if there's anything I can do to incentivize you to review future pull-requests

I don't think there is anything specific. I'm not that involved in the PeerJS project. I don't use it too much, and I don't know the codebase and the domain well, so I probably won't be the most productive person for most of the MRs.