Feature/refactor and add tests and coverage #60

ddimaria · 2022-11-18T00:19:24Z

Also, fixes #53

ddimaria · 2022-11-18T00:20:11Z

src/error.ts

@@ -1,13 +1,6 @@
 import errCode from 'err-code'


I just alpha sorted the errors

ddimaria · 2022-11-18T00:20:37Z

src/error.ts

-  constructor (streamLabel: string, errorMessage: string) {
-    super(`[stream: ${streamLabel}] data channel error: ${errorMessage}`)
-    this.name = 'WebRTC/DataChannelError'
+export class OverStreamLimitError extends WebRTCTransportError {


Renamed this for consistency

src/transport.ts

src/error.ts

…nd-add-tests-and-coverage

codecov-commenter · 2022-12-01T22:51:55Z

Codecov Report

❗ No coverage uploaded for pull request base (develop@b2e4f60). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files

@@             Coverage Diff             @@
##             develop       #60   +/-   ##
===========================================
  Coverage           ?   100.00%           
===========================================
  Files              ?         9           
  Lines              ?         9           
  Branches           ?         0           
===========================================
  Hits               ?         9           
  Misses             ?         0           
  Partials           ?         0

Flag	Coverage Δ
chrome	`100.00% <0.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

ckousik

This looks good from a preliminary read. I will review it again shortly.

ddimaria · 2022-12-02T14:55:29Z

examples/go-libp2p-server/go.mod

+
+go 1.18
+
+replace github.com/libp2p/go-libp2p => github.com/ckousik/go-libp2p v0.23.3-0.20221029141116-67b7c6290b11


We will need to update this once the Go PR is merged. I'll add an issue for it.

Link to the issue in a comment perhaps with a big fat TODO?

GlenDC · 2022-12-03T08:27:01Z

README.md


-## Hacking
+![https://github.com/libp2p/js-libp2p-interfaces/tree/master/packages/interface-connection](https://raw.githubusercontent.com/libp2p/js-libp2p-interfaces/master/packages/interface-connection/img/badge.png)


Suggested change

![https://github.com/libp2p/js-libp2p-interfaces/tree/master/packages/interface-connection](https://raw.githubusercontent.com/libp2p/js-libp2p-interfaces/master/packages/interface-connection/img/badge.png)

[![Connection interface for libp2p](https://raw.githubusercontent.com/libp2p/js-libp2p-interfaces/master/packages/interface-connection/img/badge.png)](https://github.com/libp2p/js-libp2p-interfaces/tree/master/packages/interface-connection)

Not more accessible?

GlenDC · 2022-12-03T08:28:01Z

README.md

+```js
+interface MultiaddrConnection extends Duplex<Uint8Array> {
+  close: (err?: Error) => Promise<void>
+  remoteAddr: Multiaddr


Does it has to be writable or can it be readonly? Same question for the next property.

GlenDC · 2022-12-03T08:29:15Z

README.md


-There is also `npm run autogen` which uses ProtoBuf's protoc to populate the generated code directory `proto_ts` based on `*.proto` files in src. Don't forget to run this step before `build` any time you make a change to any of the `*.proto` files.
+class WebRTCMultiaddrConnection implements MultiaddrConnection { }


Is there any value to this example as it:

neither implements the connection

nor does it get past a decent linter for several reasons

Just asking as people do tend to copy examples as is.

GlenDC · 2022-12-03T08:31:13Z

examples/browser-to-server/README.md

+npm run go-libp2p-server
+```
+
+Copy the multiaddress in the output.


might be useful to show an example of such output and multiaddress

examples/browser-to-server/README.md

GlenDC · 2022-12-03T08:33:02Z

examples/browser-to-server/README.md

+
+The browser window will automatically open.
+Using the copied multiaddress from the Go server, pasted that value into the `Server MultiAddress` input and click the `Connect` button.
+Once the peer is connected, click the message section will appear.  Enter a message and click the `Send` button.


Suggested change

Once the peer is connected, click the message section will appear. Enter a message and click the `Send` button.

Once the peer is connected, click the message section once it appeared. Then enter a message and click the `Send` button.

?

GlenDC · 2022-12-03T08:33:52Z

examples/browser-to-server/index.js

+import { fromString, toString } from "uint8arrays";
+import { webRTC } from 'js-libp2p-webrtc'
+
+  let stream;


Is this indention on purpose? o.O

Or did you forget to define (async () => { on top?

Even better, probably only want to do it once you know the DOM is initialised/ready?

GlenDC · 2022-12-03T08:34:48Z

examples/browser-to-server/index.js

+    connectionEncryption: [() => new Noise()],
+  });
+
+  await node.start()


Does this even run? Since when can you have await in a non-async body, which the global body is AFAIK?

GlenDC · 2022-12-03T08:37:29Z

examples/browser-to-server/package.json

+  "type": "module",
+  "scripts": {
+    "start": "vite",
+    "go-libp2p-server": "cd ../go-libp2p-server && go build && ./go-libp2p-server"


does npm run your scripts in a new shell? As otherwise this command would change your CWD implicitly, which might be confusing and not expected when the cmd exits?

examples/go-libp2p-server/main.go

GlenDC · 2022-12-03T08:40:51Z

examples/go-libp2p-server/main.go

+		libp2p.Ping(true),
+	)
+	if err != nil {
+		panic(err)


given you already use log pkg anyway, you might perhaps want to use log.Fatal(err) instead?

GlenDC · 2022-12-03T08:43:59Z

src/maconn.ts

+
+  /**
+   * The multiaddr address used to communicate with the remote peer
+   */
  remoteAddr: Multiaddr;


does this property and the ones below have to be public? If so, do they have to writable? If so, can they be public only as a get(ter)?

GlenDC · 2022-12-03T08:44:33Z

src/maconn.ts

-    log.error('error closing connection', err)
+    if (err !== undefined) {
+      log.error('error closing connection', err)
+    }


might be nice to have a non-err log.debug equivalent for sanity checking while debugging?

src/muxer.ts

GlenDC · 2022-12-03T08:45:40Z

src/muxer.ts

  readonly protocol: string = '/webrtc'
+
+  /**
+   * Array of streams in the data channel


Any streams? Just the ones alive? Or once in the array always in the array?

GlenDC · 2022-12-03T08:47:07Z

src/options.ts

-  //   channelOptions?: WebRTCReceiverInit
-}
-
+export interface WebRTCListenerOptions extends CreateListenerOptions {}


my linters do not accept such empty extends, I believe better practice is something like:

export { CreateListenerOptions as WebRTCListenerOptions } from '@libp2p/interface-transport'

GlenDC · 2022-12-03T08:48:36Z

src/sdp.ts

@@ -76,7 +86,9 @@ export function toSupportedHashFunction (name: multihashes.HashName): string {
  }
 }

-// Convert a multiaddr into a SDP
+/**
+ * Convert a multiaddr into a SDP


Might be nice to write fully what SDP means as <full name> (SDP), small effort but a cognitive ease of mind

GlenDC · 2022-12-03T08:50:17Z

src/sdp.ts

 export function fromMultiAddr (ma: Multiaddr, ufrag: string): RTCSessionDescriptionInit {
  return {
    type: 'answer',
    sdp: ma2sdp(ma, ufrag)
  }
 }

-// Replace the ufrag and password values in a SDP
+/**
+ * Replace (munge) the ufrag and password values in a SDP


Might just be me reading functions that I require pre-knowledge for, but I stil feels there is a lot of guess work required on my end to get what this function does without reading the actual implementation.

GlenDC · 2022-12-03T08:50:49Z

src/stream.ts

@@ -12,6 +12,9 @@ import * as pb from '../proto_ts/message.js'

 const log = logger('libp2p:webrtc:stream')

+/**
+ * Constructs a default StreamStat


might be nice to expand a bit on what these defaults entail?

src/transport.ts

GlenDC · 2022-12-03T08:54:23Z

src/transport.ts

  async dial (ma: Multiaddr, options: WebRTCDialOptions): Promise<Connection> {
    const rawConn = await this._connect(ma, options)
    log(`dialing address - ${ma.toString()}`)
    return rawConn
  }

+  /**
+   * Create transport listeners no supported by browsers


this sentence reads a big ambiguous and unclear, can you expand it please? E.g. Creating transports listeners is not supported when used from within a browser, or something like that

GlenDC · 2022-12-03T08:57:44Z

test/maconn.browser.spec.ts

+import { WebRTCMultiaddrConnection } from './../src/maconn'
+
+describe('Multiaddr Connection', () => {
+  it('can open and close', async () => {


imho it doesn't really test that right? I mean it proves the close property changes when you call close() but it might as well be done using a dummy property which toggles.

Not better to next to what you already do, also write to it, and have it succeed proving you receive the value, with then the the write or so afterwards doesn't work and you instead get some specific error that indicates the conn is closed and that you also confirm the content is indeed not written.

And same for or instead of the reading part?

GlenDC · 2022-12-03T08:58:15Z

test/maconn.browser.spec.ts

+    const maConn = new WebRTCMultiaddrConnection({
+      peerConnection: peerConnection,
+      remoteAddr,
+      timeline: {


why not make this the default value, seems like a sane default if you ask me? Would allow you to omit it here as well

GlenDC · 2022-12-03T08:59:00Z

test/stream.browser.spec.ts

@@ -1,74 +1,68 @@
 import * as underTest from '../src/stream'
 import { expect, assert } from 'chai'

+function setup (): { peerConnection: RTCPeerConnection, datachannel: RTCDataChannel, webrtcStream: underTest.WebRTCStream } {
+  const peerConnection = new RTCPeerConnection()
+  const datachannel = peerConnection.createDataChannel('whatever', { negotiated: true, id: 91 })


why not make the data channel id optional and generate a random ID by default? Seems better to me than having to invent strings like this?

GlenDC

Functionality it looks ok, even though that's harder for me to tell given I Am not yet familiar with libp2p and its specs. So my review is more generic, in that context I Requested some changes but feel free to ignore them all, as none of my comments indicate a functional flaw AFAIC

Co-authored-by: Glen De Cauwsemaecker <glen.decauwsemaecker@otainsight.com>

github-actions · 2022-12-13T18:33:49Z

🎉 This PR is included in version 1.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

ddimaria added 2 commits November 17, 2022 17:13

Fill in code documentation

c8c329e

Ignore coverage reports

6d7c528

ddimaria commented Nov 18, 2022

View reviewed changes

src/error.ts

@@ -1,13 +1,6 @@

import errCode from 'err-code'

Copy link

Collaborator Author

ddimaria Nov 18, 2022

Choose a reason for hiding this comment

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

I just alpha sorted the errors

ddimaria commented Nov 18, 2022

View reviewed changes

src/transport.ts Show resolved Hide resolved

GlenDC reviewed Nov 21, 2022

View reviewed changes

src/error.ts Outdated Show resolved Hide resolved

GlenDC approved these changes Nov 21, 2022

View reviewed changes

ddimaria added 5 commits November 30, 2022 09:43

Test maconn, update existing test and code to accomodate tests

37794cd

Create a browser-to-server example

232d171

Fix copy/paste documentation errors

1c7b463

Remove unused

4599d33

Test chrome and firefox

041a057

ddimaria marked this pull request as ready for review December 1, 2022 21:05

ddimaria added 3 commits December 1, 2022 14:07

Merge remote-tracking branch 'origin/develop' into feature/refactor-a…

d42e613

…nd-add-tests-and-coverage

Add webworker tests

86dd1d2

Removing webworker scripts as webrtc doesn't work in web/service workers

3b78322

Polish all the READMEs

7d83c81

ddimaria requested review from ckousik, John-LittleBearLabs and plauche December 2, 2022 01:00

Prefer template literals

57a0635

ckousik reviewed Dec 2, 2022

View reviewed changes

ddimaria added 2 commits December 2, 2022 07:36

Remove webworker and electron jobs in CI

581ca72

Cosmetic refactors

9d3afb0

ddimaria commented Dec 2, 2022

View reviewed changes

Update master reference to main in CI

0a6ce42

ddimaria requested a review from GlenDC December 2, 2022 15:04

ddimaria added 2 commits December 2, 2022 08:16

Refactor stream tests

584c6fa

Complete transport tests

a1df8c3