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.

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

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

ProPablo
Copy link

@ProPablo ProPablo commented Sep 19, 2023

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

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

Choose a reason for hiding this comment

The 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

Copy link
Member

Choose a reason for hiding this comment

The 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.

@@ -13,10 +26,11 @@ export interface PeerJSOption {
secure?: boolean;
token?: string;
config?: RTCConfiguration;
debug?: number;
debug?: LogLevel;
Copy link
Member

Choose a reason for hiding this comment

The 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}`

Copy link
Author

Choose a reason for hiding this comment

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

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

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

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


export class MsgPackPeer extends Peer {
override _serializers: SerializerMapping = {
MsgPack,
...defaultSerializers,
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

The 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 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.

package.json Outdated
@@ -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",
Copy link
Member

Choose a reason for hiding this comment

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

Should be added to dev-dependencies.

lib/peer.ts Outdated
Comment on lines 496 to 497
// options,
{},
Copy link
Member

Choose a reason for hiding this comment

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

Why not pass options to the dataconnection?

lib/peer.ts Outdated
);
console.log({ options, dataConnection});
Copy link
Member

Choose a reason for hiding this comment

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

To be removed before merging.

Comment on lines 87 to 88
cbor: Cbor,
msgpack: MsgPack,
Copy link
Member

Choose a reason for hiding this comment

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

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.

pnpm-lock.yaml Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Let’s put this into .gitignore.

lib/cborPeer.ts Outdated

export class CborPeer extends Peer {
override _serializers: SerializerMapping = {
Cbor,
...defaultSerializers,
Copy link
Member

Choose a reason for hiding this comment

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

See MsgPackPeer

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

Choose a reason for hiding this comment

The 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.

@jonasgloning
Copy link
Member

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
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants