-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Conversation
Nice work, this looks to have been pretty painful to put together. I'm not 100% on including types for our dependencies as we don't want to be guardians of the APIs of those modules, but I don't know enough about TS to know if that's a dealbreaker or not.
Please don't! Take some time to unwind. |
@@ -66,9 +93,11 @@ module.exports = ({ pinManager, gcLock, dag }) => { | |||
const release = await gcLock.readLock() | |||
|
|||
try { | |||
await pinAdd() | |||
return await pinAdd() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this return
here is intended behavior, given that on line 90 there's return pinAdd()
. At least that is what TS identifying as type mismatch.
const keys = ipns.getIdKeys(peerId.toBytes()) | ||
|
||
await this._publishEntry(keys.routingKey, embedPublicKeyRecord || record, peerId) | ||
await this._publishEntry(keys.routingKey, embedPublicKeyRecord || record) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_publishEntry
below takes just two arguments. I changed this because TS is rightfully points that out.
encodeBase32: (buf) => { | ||
const m = multibase.encode('base32', buf).slice(1) // slice off multibase codec | ||
|
||
return m.toString().toUpperCase() // should be uppercase for interop with go | ||
}, | ||
validator: { | ||
func: (key, record, cb) => ipns.validator.validate(record, key, cb) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validator doesn't take cb
(anymore) so I changed this to reflect current state.
@@ -4,29 +4,58 @@ const toUri = require('multiaddr-to-uri') | |||
const debug = require('debug') | |||
const CID = require('cids') | |||
const shuffle = require('array-shuffle') | |||
const AbortController = require('abort-controller') | |||
const { AbortController } = require('abort-controller') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was necessary because:
- TS does not treat ES default export as
module.exports
in CommonJS. Which on it's own would not have being issues, but for a fact that abort-controller module provides.d.ts
file that provides type signature that tells TS it is an ES module. In the end just using destracturing here addresses everything as module happens to export same value asAbortController
as well.
I'm getting inconsistent results from testing. Is there some shared knowledge I could draw from ? |
I had discussed this work with @hugomrdias on the call. We have decided that I'll leave this as is (even though it is tempting to resolve last 81 errors). He will incorporate work from here into #2945 some form. |
I have spend time trying to make progress on #2945. This is not complete TS right now reports
3211
errors which is a lot but it was 100x that when I started. I do not know how much more time I can afford spending on this, but I'll try some over the weekend.Few high level notes / overview
tsconfig.json
which I beleieven technically isn't necessary, however it is virtually impossible to otherwise understand what TS will infer. E.g. there weer cases here and dependencies where jsdoc comments existed by TSC was not happy due to various issues.types/*
directory with typings for supporting libraries & TS know to look there first before trying to look intonode_modules
.default export
does not correspond tomodule.exports
. That is also why I changedB = require(...)
to{B} = require(...)
here and there as it resolved some of those issues.types/ipfs-interface
with bunch of helper types and things so they could be pulled as needed throughout.ApiManager
has a fieldapi
who's type dependents on later call toupdate
meaning it would start of with typeT
and later change to typeD
. That is not possible I'm afraid although there are some alternatives that might be worth exploring that would enable that e.g. instead of usingapiManager.api
use{api} = apiManager.update(...)
. That can be inferred because return type could be derived from type of input.log.error = debug(...)
. To address this specific one I have created helperdebug
module that just does that inform TS about return type.withTimeoutOption(async function put (block, options) {})
do not get associated withput
function being decorated. Solution is to first define than decorate.*
are used. Which essentially tells TS to infer the type rather than complain about knowing. That is workaround to get to state where TS can successful type check. Once there we can incrementally replace those with actual types.https://www.typescriptlang.org/docs/handbook/jsdoc-supported-types.html