Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

WIP: Adding JSDoc supporrted types #3046

Closed
wants to merge 8 commits into from
Closed

Conversation

Gozala
Copy link
Contributor

@Gozala Gozala commented May 22, 2020

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

  • This does add 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.
  • I have created types/* directory with typings for supporting libraries & TS know to look there first before trying to look into node_modules.
  • There are problems with how ES modules are interpreted by CommonJS modules and vise versa mostly because default export does not correspond to module.exports. That is also why I changed B = require(...) to {B} = require(...) here and there as it resolved some of those issues.
  • I have created types/ipfs-interface with bunch of helper types and things so they could be pulled as needed throughout.
  • Dependency injection complicates things, because usually type of actual component ends enclosed in function so you can't refer to types to it from anywhere (without retyping). I do not yet know what would be a better strategy . So far my hope was that TS would be enable to infer the whole type. But even then problem is that some of that needs to be injected elsewhere and ideally dependencies would be typed.
  • There are cases where current API is fundamentally incompatible with static typing. E.g. ApiManager has a field api who's type dependents on later call to update meaning it would start of with type T and later change to type D. 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 using apiManager.api use {api} = apiManager.update(...). That can be inferred because return type could be derived from type of input.
  • There are other cases of changing types e.g. log.error = debug(...). To address this specific one I have created helper debug module that just does that inform TS about return type.
  • All over the place I had to change the way decorators functions are applied because JSDoc comments on top of e.g. withTimeoutOption(async function put (block, options) {}) do not get associated with put function being decorated. Solution is to first define than decorate.
  • There are many cases where types like * 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.
  • Some types are probably wrong. There is a lot of new code for me a I did my best to represent interfaces consulting docs were available to get things right, but I'm I made mistakes, in fact I discovered many such mistakes as TS was able to make more sense of it all. I also end fixing them where I could.

https://www.typescriptlang.org/docs/handbook/jsdoc-supported-types.html

@achingbrain
Copy link
Member

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.

I'll try some over the weekend.

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()
Copy link
Contributor Author

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

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

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')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was necessary because:

  1. 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 as AbortController as well.

@Gozala
Copy link
Contributor Author

Gozala commented May 25, 2020

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.

I would frame it as TS wants you to encode invariants you happen to assume, but it's up to you to choose the level of details. In fact types for most dependencies were generated and are really loose (have any type), which is basically tells TS don't worry about that it's all cool. There are however few modules that I've deliberately typed manually for one of the following reasons:

  1. It's library part of IPFS which happend to live in different repo. Ideally I would just added JSDoc comments there, however to reduce coordination overhead I've just created type definition in hope that later they could be moved to actual repo as JSDoc comments after which type definition could be removed.
  2. It is an essential library e.g. Without knowledge of it-pipe type signature TS (and if I dare say myself) has little clue of what are the things passed around (as illustrated by the screenshot below). This also means I need to add jsdoc comments to the closure.
    image
    With type information for pipe however TS can infer the type of source and tools like VSCode can provide helpful tips as shown in screenshot below:
    image
    I should say it is possible to suppress TS from complaining about implicit any type, however it would not be able to help (someone new to the code base, like myself) to understand what are the things being passed around without taking side trips to pipe, normaliseAddInput, importer etc...

@Gozala
Copy link
Contributor Author

Gozala commented May 25, 2020

I'm getting inconsistent results from testing. Is there some shared knowledge I could draw from ?

@Gozala
Copy link
Contributor Author

Gozala commented May 27, 2020

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.

@Gozala Gozala closed this Jul 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants