feat: Adding serialization types relevant to the new Dataconnection classes #1135

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

lib/optionInterfaces.ts

+ * 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

lib/optionInterfaces.ts

@@ -13,10 +26,11 @@ export interface PeerJSOption {
 secure?: boolean;
 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

lib/msgPackPeer.ts


 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

package.json

@@ -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",


Should be added to dev-dependencies.

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

lib/peer.ts

+ // options,
+ {},


Why not pass options to the dataconnection?

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

lib/peer.ts

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


To be removed before merging.

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

lib/optionInterfaces.ts

+ 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

pnpm-lock.yaml

Let’s put this into .gitignore.

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

lib/cborPeer.ts


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


See MsgPackPeer

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

lib/optionInterfaces.ts

+ * 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Adding serialization types relevant to the new Dataconnection classes #1135

feat: Adding serialization types relevant to the new Dataconnection classes #1135

ProPablo commented Sep 19, 2023 •

edited

ProPablo Sep 19, 2023

jonasgloning Sep 23, 2023

jonasgloning Sep 23, 2023

ProPablo Sep 24, 2023

jonasgloning Sep 23, 2023

ProPablo Sep 24, 2023

jonasgloning Sep 23, 2023

jonasgloning Sep 23, 2023

jonasgloning Sep 23, 2023

jonasgloning Sep 23, 2023

jonasgloning Sep 23, 2023

jonasgloning Sep 23, 2023

jonasgloning Sep 23, 2023

jonasgloning commented Sep 23, 2023

ProPablo commented Sep 24, 2023

feat: Adding serialization types relevant to the new Dataconnection classes #1135

Are you sure you want to change the base?

feat: Adding serialization types relevant to the new Dataconnection classes #1135

Conversation

ProPablo commented Sep 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonasgloning commented Sep 23, 2023

ProPablo commented Sep 24, 2023

ProPablo commented Sep 19, 2023 •

edited