feat: typedef generation & type checking #261

Gozala · 2020-10-14T06:38:24Z

This pull request makes following changes:

Adds typedef generation
Adds github action to run typecheck on changes & report missmatches in pull requests
Adds / fixes bunch of jsdoc comments so it can both typecheck & generate

While making these changes I did discover certain things that are either bugs or I am misunderstanding what is going on there. I'll comment inline.

Please not that this change depends on:

Adds TS typedefs so typed projects can inference pgte/moving-average#9
fix: bitswap related typedefs js-ipfs#3580 (without this type check will fail)

welcome · 2020-10-14T06:38:26Z

Thank you for submitting this PR!
A maintainer will be here shortly to review it.
We are super grateful, but we are also overloaded! Help us by making sure that:

The context for this PR is clear, with relevant discussion, decisions
and stakeholders linked/mentioned.
Your contribution itself is clear (code comments, self-review for the
rest) and in its best form. Follow the code contribution
guidelines
if they apply.

Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment.
Next steps:

A maintainer will triage and assign priority to this PR, commenting on
any missing things and potentially assigning a reviewer for high
priority items.
The PR gets reviews, discussed and approvals as needed.
The PR is merged by maintainers when it has been approved and comments addressed.

We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution.
We are very grateful for your contribution!

Gozala · 2020-10-14T06:44:53Z

src/decision-engine/index.js

@@ -112,7 +123,7 @@ class DecisionEngine {

    // If there's nothing in the message, bail out
    if (msg.empty) {
-      this._requestQueue.tasksDone(peerId, tasks)
+      peerId && this._requestQueue.tasksDone(peerId, tasks)


peerId is obtained on line 76 via following code:

const { peerId, tasks, pendingSize } = this._requestQueue.popTasks(this._opts.targetMessageSize)

However popTasks returns an object without peerId in 2 code paths out of 3. At the same time tasksDone and messageSent below do not expect undefined.

It was not exactly clear what made most sense here, so I just conditioned those calls on peerIds presence, but it would be good to make sure all this makes sense.

Gozala · 2020-10-14T06:47:10Z

src/decision-engine/req-queue.js

   * @returns {boolean}
   */
  isIdle () {
-    return this._pending.length === 0 && this._active.length === 0
+    return this._pending.length === 0 && this._active.size === 0


It looks like there was a bug in the code, because _active is a Set instance which doesn't have length property.

Gozala · 2020-10-14T06:55:12Z

src/stats/stat.js

  _applyOp (op) {
    const key = op[0]
    const inc = op[1]

    if (typeof inc !== 'number') {
-      throw new Error('invalid increment number:', inc)
+      throw new Error(`invalid increment number: ${inc}`)


This seemed like a bug, so I just inlined incinto error message.

Gozala · 2020-10-14T07:01:01Z

src/types/message/index.js

  equals (other) {
    if (this.full !== other.full ||
        this.pendingBytes !== other.pendingBytes ||
        !isMapEqual(this.wantlist, other.wantlist) ||
        !isMapEqual(this.blocks, other.blocks) ||
+        // @TODO - Is this a bug ? `isMapEqual` only compare maps of blocks and
+        // wants
+        // @ts-ignore


This one seems like a bug, but maybe it is not 🙄 So isMapEqual will compare only that keys match and will compare values if they are either Blocks or BitswapMessageEntrys (anything that has equals method on them actually). However blockPresences contain numbers so two would be equal even if the actual values are different, which doesn't seems like intended behavior.

P.S.: TS not happy mostly because it's told to expect maps of blocks or entries and is passed maps of numbers below.

Gozala · 2020-10-14T07:04:08Z

src/want-manager/msg-queue.js

  constructor (selfPeerId, otherPeerId, network) {
    this.peerId = otherPeerId
    this.network = network
    this.refcnt = 1

    this._entries = []
-    this._log = logger(selfPeerId, 'msgqueue', otherPeerId.toB58String().slice(0, 8))
+    this._log = logger(selfPeerId, 'msgqueue')


Not sure what to do with the last argument here, because logger seems to take just two. I dropped it to retain the current behavior, but maybe concatenating last two would make more sense.

hugomrdias

can we make an issue for the all ts-ignore's ?

.eslintrc

.github/workflows/typecheck.yml

package.json

src/stats/index.js

src/stats/stat.js

src/types.ts

tsconfig.json

package.json

Gozala · 2020-11-27T20:32:26Z

@hugomrdias I have addressed all of your feedback except:

Migration from travis to github actions. I don't think it makes sense to bundle it with changes here & separate PR would make more sense IMO.
AsyncIterable vs AsyncGenerator seems like a broader discussion to have.

Gozala · 2020-11-27T20:49:47Z

Merging master introduced the problem with libp2p typedefs so temporarily this pull depends on libp2p/js-libp2p-interfaces#73

Gozala · 2020-11-27T20:53:19Z

can we make an issue for the all ts-ignore's ?

#273

Gozala · 2020-11-27T20:55:20Z

@hugomrdias any idea what is going on here ?
https://travis-ci.com/github/ipfs/js-ipfs-bitswap/jobs/449964224

For some reason typescript dependency does not get hoisted in the CI, while it does seem to get hoisted on my machine.

hugomrdias · 2020-11-30T10:45:06Z

dunno maybe because some deps arent using aegir ts yet or the eslint config

hugomrdias · 2020-12-09T11:27:03Z

package.json

-    "uuid": "^8.0.0"
+    "uuid": "^8.0.0",
+    "@types/debug": "^4.1.5",
+    "typescript": "^4.0.5"


Suggested change

"typescript": "^4.0.5"

Gozala · 2020-12-17T19:29:05Z

package.json

  "files": [
    "dist",
    "src"
  ],
  "scripts": {
+    "prepare": "aegir build",


We don't need full build here

Suggested change

"prepare": "aegir build",

"prepare": "aegir ts -p check",

Gozala · 2021-03-04T22:05:03Z

src/network.js

      cid,
      {
+        // TODO: Should this be a timeout options insetad ?


@vasco-santos do you know if we should be passing timeout instead of maxTimeout

This should be timeout - the option is passed through to whatever contenting routing modules are configured - these are either libp2p-delegated-content-routing or libp2p-kad-dht and neither of those support a maxTimeout option, only timeout.

Gozala · 2021-03-04T22:09:01Z

src/network.js

   * @returns {Promise<Connection>}
   */
  async connectTo (peer, options) { // eslint-disable-line require-await
    if (!this._running) {
      throw new Error('network isn\'t running')
    }

-    return this.libp2p.dial(peer, options)
+    // TODO: Figure out inconsistency here.


@vasco-santos could you please help me out understand what is expected here. Passed peer here comes from libp2p.contentRouting.findProviders which contains both peer id and multiaddr, but then libp2p.dial expects either peerId or multiaddr and attempts to turn it back into Provider.

I do not know all the ins and outs here, but clearly something makes wrong assumptions.

libp2p.contentRouting.findProviders should return only PeerIDs.

The idea is that libp2p manages the actual multiaddrs internally and we don't worry about them at this level.

Created an issue in libp2p libp2p/js-libp2p#899

Gozala · 2021-03-04T22:12:00Z

src/types/message/index.js

  equals (other) {
    if (this.full !== other.full ||
        this.pendingBytes !== other.pendingBytes ||
        !isMapEqual(this.wantlist, other.wantlist) ||
        !isMapEqual(this.blocks, other.blocks) ||
+        // @TODO - Is this a bug ?
+        // @ts-expect-error - isMap equals map values to be objects not numbers


@hugomrdias This looks like a bug to me, could you please confirm or tell me if I'm not seeing something ?

Gozala · 2021-03-04T22:15:12Z

src/want-manager/index.js

        } else {
          this.wantlist.remove(e.cid)
        }
      } else {
        this._log('adding to wl')
+        // TODO: Figure out the wantType
+        // @ts-expect-error - requires wantType
        this.wantlist.add(e.cid, e.priority)


@hugomrdias do you understand why wantType is omitted here ? Following lines make make me believe it should be provided, but I'm not sure what the value should be in this instance

js-ipfs-bitswap/src/types/wantlist/index.js

Lines 28 to 32 in 681b015

if (entry.wantType === Message.WantType.Have && wantType === Message.WantType.Block) {

entry.wantType = wantType

}

} else {

this.set.set(cidStr, new Entry(cid, priority, wantType))

I think the WantType ends up defaulting to Block if it's omitted. The Have/Block distinction is a later optimisation, I think maybe it just got missed off here when it was implemented.

Gozala · 2021-03-04T22:19:11Z

src/types/wantlist/index.js

@@ -72,9 +97,16 @@ class Wantlist {
  }

  sortedEntries () {
+    // TODO: Figure out if this is an actual bug.
+    // @ts-expect-error - Property 'key' does not exist on type 'WantListEntry'


@achingbrain do you have an idea what this meant to be, I'm guessing key used to be what cid is today, but I'm not sure. Nor I have a clue how this supposed to be sorted.

Looks like a bug. I think this is just supposed to be a list of wantlist entries sorted by string CID, rather than insertion order which would be the default - the .key property access is probably due to misreading the docs on the output of Map.entries()?

I'm not sure what the value of it being sorted is, other than the output being stable - it's probably better as a set TBH.

Something we should create an issue for and maybe address later.

achingbrain

LGTM - a few bugs and inconsistencies are well pointed out but we should get this in and address those as follow ups.

Just needs the github URLs removing from deps and it's good to go.

package.json

src/index.js

achingbrain · 2021-03-08T11:35:06Z

src/network.js

      cid,
      {
+        // TODO: Should this be a timeout options insetad ?


This should be timeout - the option is passed through to whatever contenting routing modules are configured - these are either libp2p-delegated-content-routing or libp2p-kad-dht and neither of those support a maxTimeout option, only timeout.

achingbrain · 2021-03-08T11:37:08Z

src/network.js

-   * @param {Object} options
-   * @param {AbortSignal} options.abortSignal
-   * @returns {void}
+   * @param {Object} [options]


Small nit and not a blocker but in this PR I see @param {Object} in some places and @param {object} in others.

I know it makes no difference functionally, but we should be consistent (and it should really be enforced by our linting rules). I think we're using @param {object} everywhere else.

achingbrain · 2021-03-08T11:39:36Z

src/network.js

   * @returns {Promise<Connection>}
   */
  async connectTo (peer, options) { // eslint-disable-line require-await
    if (!this._running) {
      throw new Error('network isn\'t running')
    }

-    return this.libp2p.dial(peer, options)
+    // TODO: Figure out inconsistency here.


libp2p.contentRouting.findProviders should return only PeerIDs.

The idea is that libp2p manages the actual multiaddrs internally and we don't worry about them at this level.

achingbrain · 2021-03-08T12:18:21Z

src/types/wantlist/index.js

@@ -72,9 +97,16 @@ class Wantlist {
  }

  sortedEntries () {
+    // TODO: Figure out if this is an actual bug.
+    // @ts-expect-error - Property 'key' does not exist on type 'WantListEntry'


Looks like a bug. I think this is just supposed to be a list of wantlist entries sorted by string CID, rather than insertion order which would be the default - the .key property access is probably due to misreading the docs on the output of Map.entries()?

I'm not sure what the value of it being sorted is, other than the output being stable - it's probably better as a set TBH.

Something we should create an issue for and maybe address later.

achingbrain · 2021-03-08T12:26:33Z

src/want-manager/index.js

        } else {
          this.wantlist.remove(e.cid)
        }
      } else {
        this._log('adding to wl')
+        // TODO: Figure out the wantType
+        // @ts-expect-error - requires wantType
        this.wantlist.add(e.cid, e.priority)


I think the WantType ends up defaulting to Block if it's omitted. The Have/Block distinction is a later optimisation, I think maybe it just got missed off here when it was implemented.

Fixes up types so they align here ipfs/js-ipfs-bitswap#261 Co-authored-by: achingbrain <alex@achingbrain.net>

Gozala · 2021-03-10T18:03:47Z

Created an issue for all the discovered inconsistencies #303

feat: typedef generation & type checking

72cf949

Gozala requested a review from achingbrain October 14, 2020 06:38

Gozala commented Oct 14, 2020

View reviewed changes

fix: fill type holes

0ca4e3d

Gozala commented Oct 14, 2020

View reviewed changes

Gozala requested a review from hugomrdias November 18, 2020 17:38

hugomrdias suggested changes Nov 26, 2020

View reviewed changes

Gozala commented Nov 26, 2020

View reviewed changes

package.json Outdated Show resolved Hide resolved

chore: address review comments

c028b3c

Gozala requested a review from hugomrdias November 27, 2020 20:23

chore: merge master

80b8bed

Gozala mentioned this pull request Nov 27, 2020

fix: typedefs for MulticodecTopology libp2p/js-libp2p-interfaces#73

Closed

fix: regression in typings

1c84c82

hugomrdias mentioned this pull request Nov 30, 2020

Track: Typescript types support ipfs/js-ipfs#2945

Closed

chore: retrigger ci with changed deps

1afa39d

Gozala force-pushed the typegen branch from f40d293 to 1afa39d Compare December 1, 2020 06:47

fix: add ts to dev-deps

49d0998

hugomrdias approved these changes Dec 9, 2020

View reviewed changes

achingbrain added 2 commits December 17, 2020 18:20

chore: update deps, type tests as well as source

815bcbd

chore: update typecheck version

bd9d4db

Gozala commented Dec 17, 2020

View reviewed changes

Gozala mentioned this pull request Mar 4, 2021

fix: bitswap related typedefs ipfs/js-ipfs#3580

Merged

Gozala added 2 commits March 4, 2021 13:45

chore: update to new aegir

e51719e

chore: merge master

aa49791

Gozala commented Mar 4, 2021

View reviewed changes

achingbrain requested changes Mar 8, 2021

View reviewed changes

achingbrain added a commit to ipfs/js-ipfs that referenced this pull request Mar 8, 2021

fix: bitswap related typedefs (#3580)

1af82d1

Fixes up types so they align here ipfs/js-ipfs-bitswap#261 Co-authored-by: achingbrain <alex@achingbrain.net>

achingbrain added 4 commits March 9, 2021 10:56

chore: remove gh urls

9a0708c

chore: update node version

76036fa

chore: update bundle size config param

71c3d15

chore: add temporary exit

bdcc596

achingbrain merged commit fca78c8 into master Mar 9, 2021

achingbrain deleted the typegen branch March 9, 2021 12:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: typedef generation & type checking #261

feat: typedef generation & type checking #261

Gozala commented Oct 14, 2020 •

edited

Loading

welcome bot commented Oct 14, 2020

Gozala Oct 14, 2020

Gozala Oct 14, 2020

Gozala Oct 14, 2020

Gozala Oct 14, 2020

Gozala Oct 14, 2020

hugomrdias left a comment

Gozala commented Nov 27, 2020

Gozala commented Nov 27, 2020

Gozala commented Nov 27, 2020

Gozala commented Nov 27, 2020

hugomrdias commented Nov 30, 2020

hugomrdias Dec 9, 2020

Gozala Dec 17, 2020

Gozala Mar 4, 2021

achingbrain Mar 8, 2021

Gozala Mar 4, 2021

achingbrain Mar 8, 2021

Gozala Mar 10, 2021

Gozala Mar 4, 2021

Gozala Mar 4, 2021

achingbrain Mar 8, 2021

Gozala Mar 4, 2021

achingbrain Mar 8, 2021

achingbrain left a comment

achingbrain Mar 8, 2021

achingbrain Mar 8, 2021

achingbrain Mar 8, 2021

achingbrain Mar 8, 2021

achingbrain Mar 8, 2021

Gozala commented Mar 10, 2021

	if (entry.wantType === Message.WantType.Have && wantType === Message.WantType.Block) {
	entry.wantType = wantType
	}
	} else {
	this.set.set(cidStr, new Entry(cid, priority, wantType))

feat: typedef generation & type checking #261

feat: typedef generation & type checking #261

Conversation

Gozala commented Oct 14, 2020 • edited Loading

welcome bot commented Oct 14, 2020

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

hugomrdias left a comment

Choose a reason for hiding this comment

Gozala commented Nov 27, 2020

Gozala commented Nov 27, 2020

Gozala commented Nov 27, 2020

Gozala commented Nov 27, 2020

hugomrdias commented Nov 30, 2020

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

achingbrain left a 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

Gozala commented Mar 10, 2021

Gozala commented Oct 14, 2020 •

edited

Loading