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

feat: add typeScript support #3236

Merged
merged 21 commits into from
Sep 3, 2020
Merged

feat: add typeScript support #3236

merged 21 commits into from
Sep 3, 2020

Conversation

Xmader
Copy link
Contributor

@Xmader Xmader commented Aug 21, 2020

#2945 #1166

TypeScript support for ipfs and ipfs-http-client

Work in progress

@achingbrain achingbrain requested a review from Gozala August 24, 2020 11:08
Copy link
Contributor

@Gozala Gozala left a comment

Choose a reason for hiding this comment

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

Thanks @Xmader, I'm really excited to get this entyped and to learn some new tricks!

I have added bunch of nits & notes (signified with 💭 and 📝), feel free to choose to disregard or address them. There is one 🚨 which I would like to see addressed before merging these in. I also posted some questions where I would like to better understand some changes or motivation behind them.

@@ -1,8 +1,22 @@
'use strict'

module.exports = class ApiManager {
/**
* @callback UndefFn
* @param {PropertyKey} prop
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason why to note add @return type here and do it on line 18 instead ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

eslint valid-jsdoc requires an @return on line 18, and the default return type for @callback definition is any

packages/ipfs/src/core/components/add-all/index.js Outdated Show resolved Hide resolved
* @property {string} [path] - The path you want to the file to be accessible at from the root CID _after_ it has been added
* @property {FileContent} [content] - The contents of the file
* @property {number | string} [mode] - File mode to store the entry with (see https://en.wikipedia.org/wiki/File_system_permissions#Numeric_notation)
* @property {UnixTime} [mtime] - The modification time of the entry
Copy link
Contributor

Choose a reason for hiding this comment

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

📝 If you want to be more precise (not asking you to, I have taken same shortcuts myself) FileObject would be:

type FileObject =
  | { path: string, mode?: Mode, mtime?: UnixTime } // Directory
  | { path?: string, content: FileContent, mtime?: UnixTime } // File

That is to say it must have path or a content or both.

* @property {UnixTime} [mtime] - The modification time of the entry
*
* @typedef {FileContent | FileObject} Source
* @typedef {Iterable<Source> | AsyncIterable<Source> | ReadableStream<Source>} FileStream
Copy link
Contributor

Choose a reason for hiding this comment

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

@achingbrain now that I'm looking at it, I find FileStream to be misleading, as I expect stream of representing file contents not the stream of files. Can we rename to ImportStream or something along those lines instead ?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, I'm all for making it more straightforward.

Input streams are largely called source throughout the codebase, though I see Source here is being used for the stream contents?

* @property {number | string} [mode] - File mode to store the entry with (see https://en.wikipedia.org/wiki/File_system_permissions#Numeric_notation)
* @property {UnixTime} [mtime] - The modification time of the entry
*
* @typedef {FileContent | FileObject} Source
Copy link
Contributor

Choose a reason for hiding this comment

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

💭 I think it would be better to call this something like FileSource or FileImport so it's little bit more meaningful out of context.

@@ -1,6 +1,6 @@
'use strict'

const Big = require('bignumber.js')
const Big = require('bignumber.js').default
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change necessary ? From their website this is how they suggest to import

const BigNumber = require('bignumber.js');

So I would much rather keep it as it was, unless absolutely necessary. Otherwise it's fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To make the typings work, the export signature in its .d.ts file

export default BigNumber;
export declare class BigNumber ...

is either require('bignumber.js').default or require('bignumber.js').BigNumber in CommonJS

Copy link
Contributor

Choose a reason for hiding this comment

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

I think when namespace and class name do collide in definitions TS does know to not require default, but I could be wrong and it might be behind some config flag. I think I'd still prefer const { BigNumber: Big } = require('bignumber.js') over current version, but I'm ok with current version too.

packages/ipfs/src/core/index.js Outdated Show resolved Hide resolved
return api.start()
/**
* @template T, THEN, ELSE
* @typedef {NonNullable<T> extends false ? THEN : ELSE} IsFalse
Copy link
Contributor

Choose a reason for hiding this comment

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

Conditionals used to confuse TS via JSDoc and required \n? to work. Maybe that got fixed. Could yo please confirm that is the case.

* @param {boolean} [options.repoAutoMigrate] - `js-ipfs` comes bundled with a tool that automatically migrates your IPFS repository when a new version is available. (Default: `true`)
* @param {INIT} [options.init] - Perform repo initialization steps when creating the IPFS node. (Default: `true`)
* - Note that *initializing* a repo is different from creating an instance of [`ipfs.Repo`](https://github.com/ipfs/js-ipfs-repo). The IPFS constructor sets many special properties when initializing a repo, so you should usually not try and call `repoInstance.init()` yourself.
* @param {START} [options.start] - If `false`, do not automatically start the IPFS node. Instead, you’ll need to manually call [`node.start()`](#nodestart) yourself. (Default: `true`)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is neat, I did not know of this trick. Will have to remember for the future.

*/
/** @type {IsFalse<INIT, typeof api, IsFalse<START, typeof initializedApi, typeof startedApi>>} */
// @ts-ignore
const ipfs = startedApi || initializedApi || api
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this really bring anything beyond {typeof api} ? Unless return type will end up different, which I don't think it will, I would rather keep things simple and don't introduce conditionals here.

In fact old code returned same api ref even if init was true.

Copy link
Contributor Author

@Xmader Xmader Aug 25, 2020

Choose a reason for hiding this comment

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

IPFS.create({ init: false }) returns the api ref with only 3 methods.

console.log(IPFS.create({ init: false }))
// Promise {
//   {
//     init: [AsyncFunction: init],
//     dns: [Function],
//     isOnline: [Function]
//   }
// }

IPFS.create({ start: false }) returns with 25 methods/properties;
IPFS.create() returns with 31 methods/properties.

Copy link
Contributor

@Gozala Gozala left a comment

Choose a reason for hiding this comment

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

It still would be nice to change naming of things for clarity, but I don't think we should block this as renaming could be done in a followup.

Copy link
Member

@achingbrain achingbrain left a comment

Choose a reason for hiding this comment

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

LGTM, just the question about disabling linting for newly added jsdoc comments.

Thanks so much for submitting this!


module.exports = configure((api) => {
return async function * addAll (input, options = {}) {
// eslint-disable-next-line valid-jsdoc
Copy link
Member

Choose a reason for hiding this comment

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

Why are we knowingly adding invalid js doc comments? Is the type here (and all the following instances) something that cannot be expressed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

valid-jsdoc requires @param definitions for functions even @type presents

Copy link
Contributor Author

@Xmader Xmader Sep 2, 2020

Choose a reason for hiding this comment

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

BTW, ESLint's valid-jsdoc rule has been deprecated for long time. We should replace this.

@achingbrain achingbrain changed the title TypeScript support feat: add typeScript support Sep 3, 2020
@achingbrain achingbrain merged commit be26dd7 into ipfs:master Sep 3, 2020
achingbrain added a commit that referenced this pull request Sep 3, 2020
achingbrain added a commit that referenced this pull request Sep 3, 2020
This reverts commit be26dd7.

The build was failing.
@achingbrain
Copy link
Member

achingbrain commented Sep 3, 2020

Ah, I merged this a little too quickly - the build is failing because all the AbortController imports have changed from:

const AbortController = require('abort-controller')

to:

const { AbortController } = require('abort-controller')

which doesn't work in the browser. It needs to be:

const { default: AbortController } = require('abort-controller')

or:

const AbortController = require('abort-controller').default

or just left as:

const AbortController = require('abort-controller')

Is there any TS-related reason for changing the import style or can it be left as it was?

@Xmader
Copy link
Contributor Author

Xmader commented Sep 3, 2020

Is there any TS-related reason for changing the import style or can it be left as it was?

same as #3236 (comment),
the export signature in its .d.ts file

export default AbortController
export { AbortController, AbortSignal }

is either require('abort-controller.js').default or require('abort-controller.js').AbortController in CommonJS.

It so weird that the package's browser implementation does not match it's export signature.

@achingbrain
Copy link
Member

const AbortController = require('abort-controller').AbortController

is the same as:

const { AbortController } = require('abort-controller')

..which doesn't work, I think because the browser override file in the abort-controller module does not export anything for the AbortController key.

I can't reopen a merged PR - can you please submit a new one with the same changes + the AbortController import in a way that passes CI?

SgtPooki referenced this pull request in ipfs/js-kubo-rpc-client Aug 18, 2022
TypeScript support for `ipfs` and `ipfs-http-client`

Refs: #2945
Refs: #1166 

Co-authored-by: Irakli Gozalishvili <contact@gozala.io>
Co-authored-by: Alex Potsides <alex@achingbrain.net>
SgtPooki referenced this pull request in ipfs/js-kubo-rpc-client Aug 18, 2022
This reverts commit be26dd7.

The build was failing.
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.

3 participants