feat: Adding serialization types relevant to the new Dataconnection classes by ProPablo · Pull Request #1135 · peers/peerjs

ProPablo · 2023-09-19T21:46:08Z

Exposes LogLevel to be used by the user in a typescript setting
Adding the new Serialization types to the enum and using that enum in the relevant class
Uses npx rimraf rather than rm -rf for build command for easy compatability
(Possibly subject to change/rejection) Changing the SerializationMapping type to enforce all all types of serialization existing in the map (see comment in code) :
- One concern id raise over doing this is that it locks the user into ONLY the SerializerTypes that we provide, Changing this back to the following reverts this
```
[key: string]: new (
  	  peerId: string,
  	  provider: Peer,
  	  options: any,
    ) => DataConnection;
```
Removed Serialization.Default and instead provide a default serialization in the class.
- This is to prevent situations where the default constructor is used but the other peer doesn't have it loaded in the mapping
Error handling if the serializer is not in the mapping

An example of creating the options with a custom set of serializers would look like this:

const peerOptions: PeerOptions = {
			host: PUBLIC_HOST,
			port: parseInt(PUBLIC_PORT),
			path: PUBLIC_PEERPATH,
			debug: LogLevel.All,
			serializers: {
				cbor: Cbor,
			}
		};
let peer = new Peer(peerOptions);

ProPablo · 2023-09-19T21:47:24Z

+ * Should map 1:1 with the serialization type property in the class.
+ */
+export type SerializerMapping = {
+	[key in SerializationType]: SerializationTypeConstructor;


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

I think this is a problem. Ideally, future encoding formats can be provided as a plugin to PeerJS.

jonasgloning · 2023-09-23T20:40:35Z

 	token?: string;
 	config?: RTCConfiguration;
-	debug?: number;
+	debug?: LogLevel;


Let’s wrap this as a string, so a Typescript user is not forced to use the enum.

debug?: `${LogLevel}`

So I can convert this over to string literals like this:

Changing to suit this would mean doing this all over the application

Let me know if there is a better option or if you want to see this imlemented.

jonasgloning · 2023-09-23T20:43:22Z

 export class MsgPackPeer extends Peer {
 	override _serializers: SerializerMapping = {
-		MsgPack,
+		...defaultSerializers,


My mental model was that during a life time of a Peer, only one encoding will be used in practice.

I left out the default serializers intentionally, so that a tree-shaked MsgPackPeer does not have to include all of BinaryPack

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 Peer that contain the serializers.

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.

jonasgloning · 2023-09-23T20:49:02Z

 		"check": "tsc --noEmit && tsc -p e2e/tsconfig.json --noEmit",
 		"watch": "parcel watch",
-		"build": "rm -rf dist && parcel build",
+		"build": "npx rimraf dist && parcel build",


Should be added to dev-dependencies.

jonasgloning · 2023-09-23T20:50:39Z

+			// options,
+			{},


Why not pass options to the dataconnection?

jonasgloning · 2023-09-23T20:50:55Z

+			// options,
+			{},
 		);
+		console.log({ options, dataConnection});


To be removed before merging.

jonasgloning · 2023-09-23T20:55:01Z

+	cbor: Cbor,
+	msgpack: MsgPack,


Including all encodings will increase the bundle size enormously.
The default should just be raw, json and binary, as these are mandated by backwards compatibility.

jonasgloning · 2023-09-23T20:57:28Z

Let’s put this into .gitignore.

jonasgloning · 2023-09-23T20:58:36Z

 export class CborPeer extends Peer {
 	override _serializers: SerializerMapping = {
-		Cbor,
+		...defaultSerializers,


See MsgPackPeer

jonasgloning · 2023-09-23T21:08:48Z

+ * Should map 1:1 with the serialization type property in the class.
+ */
+export type SerializerMapping = {
+	[key in SerializationType]: SerializationTypeConstructor;


I think this is a problem. Ideally, future encoding formats can be provided as a plugin to PeerJS.

jonasgloning · 2023-09-23T21:18:57Z

Very welcome PR!

BrowserStack fails because this comes from an external fork, if you would like to contribute to PeerJS in the future, I could add you to the organization.

Please don’t be afraid to open 10 new small PRs instead of one with multiple edits. That also keeps review times short.

ProPablo · 2023-09-24T07:17:52Z

Very welcome PR!

Thank you!

BrowserStack fails because this comes from an external fork, if you would like to contribute to PeerJS in the future, I could add you to the organization.

Yes please, I would be more than glad to contribute more, I have another contribution in the works as well!

Please don’t be afraid to open 10 new small PRs instead of one with multiple edits. That also keeps review times short.

Yep yep, thank you for the advice, will follow going forward.

Leon Ahuja added 5 commits September 10, 2023 17:23

enum changes

9242da7

trying to add the enums to export

647b02e

add default and tests

ba9715b

Working changes and refactor to enums

0f1177a

allow any string as well as an option to serialization

287f04d

ProPablo commented Sep 19, 2023

View reviewed changes

jonasgloning reviewed Sep 23, 2023

View reviewed changes

jonasgloning requested changes Sep 23, 2023

View reviewed changes

Leon Ahuja added 2 commits September 24, 2023 14:03

adressing comments

d511d68

removing default serialization enum

45147b4

updated package json and package lock

d9ece0e

Uh oh!

Conversation

ProPablo commented Sep 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasgloning commented Sep 23, 2023

Uh oh!

ProPablo commented Sep 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ProPablo commented Sep 19, 2023 •

edited

Loading