-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
feat: Adding serialization types relevant to the new Dataconnection classes #1135
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
base: master
Are you sure you want to change the base?
Changes from 5 commits
9242da7
647b02e
ba9715b
0f1177a
287f04d
d511d68
45147b4
d9ece0e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,10 @@ | ||
import { Peer, type SerializerMapping } from "./peer"; | ||
import { Peer, } from "./peer"; | ||
import { Cbor } from "./dataconnection/StreamConnection/Cbor"; | ||
import { SerializerMapping, defaultSerializers } from "./optionInterfaces"; | ||
|
||
export class CborPeer extends Peer { | ||
override _serializers: SerializerMapping = { | ||
Cbor, | ||
...defaultSerializers, | ||
default: Cbor, | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,10 @@ | ||
import { Peer, type SerializerMapping } from "./peer"; | ||
import { MsgPack } from "./exports"; | ||
import { Peer, } from "./peer"; | ||
import { MsgPack} from "./exports"; | ||
import { SerializerMapping, defaultSerializers } from "./optionInterfaces"; | ||
|
||
export class MsgPackPeer extends Peer { | ||
override _serializers: SerializerMapping = { | ||
MsgPack, | ||
...defaultSerializers, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My mental model was that during a life time of a I left out the default serializers intentionally, so that a tree-shaked There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah I see, thats very fair, in that case, I think it would be more appropriate just to force the user to dependency inject the serialization mapping than to make permuations of This would mean a 'blank' Peer that contains no serializers to begin with, but im not sure how the tree shaking would work on that. |
||
default: MsgPack, | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,16 @@ | ||
import { DataConnection } from "./dataconnection/DataConnection"; | ||
import { SerializationType } from "./enums"; | ||
import { LogLevel } from "./logger"; | ||
import { Peer } from "./peer"; | ||
|
||
|
||
import { BinaryPack } from "./dataconnection/BufferedConnection/BinaryPack"; | ||
import { Raw } from "./dataconnection/BufferedConnection/Raw"; | ||
import { Json } from "./dataconnection/BufferedConnection/Json"; | ||
import { Cbor } from "./dataconnection/StreamConnection/Cbor"; | ||
import { MsgPack } from "./exports"; | ||
|
||
|
||
export interface AnswerOption { | ||
/** | ||
* Function which runs before create answer to modify sdp answer message. | ||
|
@@ -13,10 +26,11 @@ export interface PeerJSOption { | |
secure?: boolean; | ||
token?: string; | ||
config?: RTCConfiguration; | ||
debug?: number; | ||
debug?: LogLevel; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let’s wrap this as a string, so a Typescript user is not forced to use the enum. debug?: `${LogLevel}` There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
referrerPolicy?: ReferrerPolicy; | ||
} | ||
|
||
|
||
export interface PeerConnectOption { | ||
/** | ||
* A unique label by which you want to identify this data connection. | ||
|
@@ -32,7 +46,7 @@ export interface PeerConnectOption { | |
* Can be any serializable type. | ||
*/ | ||
metadata?: any; | ||
serialization?: string; | ||
serialization?: `${SerializationType}` | string; | ||
reliable?: boolean; | ||
} | ||
|
||
|
@@ -49,3 +63,29 @@ export interface CallOption { | |
*/ | ||
sdpTransform?: Function; | ||
} | ||
|
||
|
||
type SerializationTypeConstructor = new ( | ||
peerId: string, | ||
provider: Peer, | ||
options: any, | ||
) => DataConnection; | ||
|
||
/** | ||
* A mapping of serialization types to their implementations. | ||
* Should map 1:1 with the serialization type property in the class. | ||
*/ | ||
export type SerializerMapping = { | ||
[key in SerializationType]: SerializationTypeConstructor; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One concern id raise over doing this is that it locks the user into ONLY the SerializerTypes that we provide, Changing this back to [key: string]: new (
peerId: string,
provider: Peer,
options: any,
) => DataConnection; reverts this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is a problem. Ideally, future encoding formats can be provided as a plugin to PeerJS. |
||
} | ||
|
||
export const defaultSerializers: SerializerMapping = { | ||
raw: Raw, | ||
json: Json, | ||
binary: BinaryPack, | ||
"binary-utf8": BinaryPack, | ||
cbor: Cbor, | ||
msgpack: MsgPack, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Including all encodings will increase the bundle size enormously. |
||
|
||
default: BinaryPack, | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,24 +2,23 @@ import { util } from "./util"; | |
import logger, { LogLevel } from "./logger"; | ||
import { Socket } from "./socket"; | ||
import { MediaConnection } from "./mediaconnection"; | ||
import type { DataConnection } from "./dataconnection/DataConnection"; | ||
import { DataConnection } from "./dataconnection/DataConnection"; | ||
import { | ||
ConnectionType, | ||
PeerErrorType, | ||
SerializationType, | ||
ServerMessageType, | ||
SocketEventType, | ||
} from "./enums"; | ||
import type { ServerMessage } from "./servermessage"; | ||
import { API } from "./api"; | ||
import type { | ||
CallOption, | ||
PeerConnectOption, | ||
PeerJSOption, | ||
import { | ||
SerializerMapping, | ||
type CallOption, | ||
type PeerConnectOption, | ||
type PeerJSOption, | ||
defaultSerializers, | ||
} from "./optionInterfaces"; | ||
import { BinaryPack } from "./dataconnection/BufferedConnection/BinaryPack"; | ||
import { Raw } from "./dataconnection/BufferedConnection/Raw"; | ||
import { Json } from "./dataconnection/BufferedConnection/Json"; | ||
|
||
import { EventEmitterWithError, PeerError } from "./peerError"; | ||
|
||
class PeerOptions implements PeerJSOption { | ||
|
@@ -69,14 +68,6 @@ class PeerOptions implements PeerJSOption { | |
|
||
export { type PeerOptions }; | ||
|
||
export interface SerializerMapping { | ||
[key: string]: new ( | ||
peerId: string, | ||
provider: Peer, | ||
options: any, | ||
) => DataConnection; | ||
} | ||
|
||
export interface PeerEvents { | ||
/** | ||
* Emitted when a connection to the PeerServer is established. | ||
|
@@ -107,20 +98,14 @@ export interface PeerEvents { | |
*/ | ||
error: (error: PeerError<`${PeerErrorType}`>) => void; | ||
} | ||
|
||
/** | ||
* A peer who can initiate connections with other peers. | ||
*/ | ||
export class Peer extends EventEmitterWithError<PeerErrorType, PeerEvents> { | ||
private static readonly DEFAULT_KEY = "peerjs"; | ||
|
||
protected readonly _serializers: SerializerMapping = { | ||
raw: Raw, | ||
json: Json, | ||
binary: BinaryPack, | ||
"binary-utf8": BinaryPack, | ||
|
||
default: BinaryPack, | ||
}; | ||
protected readonly _serializers: SerializerMapping = defaultSerializers; | ||
private readonly _options: PeerOptions; | ||
private readonly _api: API; | ||
private readonly _socket: Socket; | ||
|
@@ -232,7 +217,6 @@ export class Peer extends EventEmitterWithError<PeerErrorType, PeerEvents> { | |
token: util.randomToken(), | ||
config: util.defaultConfig, | ||
referrerPolicy: "strict-origin-when-cross-origin", | ||
serializers: {}, | ||
...options, | ||
}; | ||
this._options = options; | ||
|
@@ -489,15 +473,15 @@ export class Peer extends EventEmitterWithError<PeerErrorType, PeerEvents> { | |
*/ | ||
connect(peer: string, options: PeerConnectOption = {}): DataConnection { | ||
options = { | ||
serialization: "default", | ||
serialization: SerializationType.Default, | ||
...options, | ||
}; | ||
if (this.disconnected) { | ||
logger.warn( | ||
"You cannot connect to a new Peer because you called " + | ||
".disconnect() on this Peer and ended your connection with the " + | ||
"server. You can create a new Peer to reconnect, or call reconnect " + | ||
"on this peer if you believe its ID to still be available.", | ||
".disconnect() on this Peer and ended your connection with the " + | ||
"server. You can create a new Peer to reconnect, or call reconnect " + | ||
"on this peer if you believe its ID to still be available.", | ||
); | ||
this.emitError( | ||
PeerErrorType.Disconnected, | ||
|
@@ -509,8 +493,10 @@ export class Peer extends EventEmitterWithError<PeerErrorType, PeerEvents> { | |
const dataConnection = new this._serializers[options.serialization]( | ||
peer, | ||
this, | ||
options, | ||
// options, | ||
{}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not pass options to the dataconnection? |
||
); | ||
console.log({ options, dataConnection}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be removed before merging. |
||
this._addConnection(peer, dataConnection); | ||
return dataConnection; | ||
} | ||
|
@@ -529,8 +515,8 @@ export class Peer extends EventEmitterWithError<PeerErrorType, PeerEvents> { | |
if (this.disconnected) { | ||
logger.warn( | ||
"You cannot connect to a new Peer because you called " + | ||
".disconnect() on this Peer and ended your connection with the " + | ||
"server. You can create a new Peer to reconnect.", | ||
".disconnect() on this Peer and ended your connection with the " + | ||
"server. You can create a new Peer to reconnect.", | ||
); | ||
this.emitError( | ||
PeerErrorType.Disconnected, | ||
|
@@ -735,7 +721,7 @@ export class Peer extends EventEmitterWithError<PeerErrorType, PeerEvents> { | |
* the cloud server, email [email protected] to get the functionality enabled for | ||
* your key. | ||
*/ | ||
listAllPeers(cb = (_: any[]) => {}): void { | ||
listAllPeers(cb = (_: any[]) => { }): void { | ||
this._api | ||
.listAllPeers() | ||
.then((peers) => cb(peers)) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -175,7 +175,7 @@ | |
"contributors": "git-authors-cli --print=false && prettier --write package.json && git add package.json package-lock.json && git commit -m \"chore(contributors): update and sort contributors list\"", | ||
"check": "tsc --noEmit && tsc -p e2e/tsconfig.json --noEmit", | ||
"watch": "parcel watch", | ||
"build": "rm -rf dist && parcel build", | ||
"build": "npx rimraf dist && parcel build", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should be added to dev-dependencies. |
||
"prepublishOnly": "npm run build", | ||
"test": "jest", | ||
"test:watch": "jest --watch", | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See
MsgPackPeer