feat: add typeScript support #3236

Xmader · 2020-08-21T04:31:27Z

TypeScript support for ipfs and ipfs-http-client

Work in progress

Gozala

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.

Gozala · 2020-08-24T22:46:05Z

packages/ipfs/src/core/api-manager.js

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

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


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

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

Gozala · 2020-08-24T22:59:10Z

packages/ipfs/src/core/components/add-all/index.js

+ * @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


📝 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.

Gozala · 2020-08-24T23:03:12Z

packages/ipfs/src/core/components/add-all/index.js

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


@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 ?

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?

Gozala · 2020-08-24T23:10:25Z

packages/ipfs/src/core/components/add-all/index.js

+ * @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


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

Gozala · 2020-08-24T23:16:56Z

packages/ipfs/src/core/components/bitswap/stat.js

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

-const Big = require('bignumber.js')
+const Big = require('bignumber.js').default


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.

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

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

Gozala · 2020-08-24T23:49:53Z

packages/ipfs/src/core/index.js

-  return api.start()
+  /**
+   * @template T, THEN, ELSE
+   * @typedef {NonNullable<T> extends false ? THEN : ELSE} IsFalse


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

Gozala · 2020-08-24T23:53:32Z

packages/ipfs/src/core/index.js

+ * @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`)


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

Gozala · 2020-08-24T23:54:34Z

packages/ipfs/src/core/index.js

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


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.

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.

Co-authored-by: Irakli Gozalishvili <[email protected]>

Gozala

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.

achingbrain

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

Thanks so much for submitting this!

achingbrain · 2020-09-02T09:23:14Z

packages/ipfs-http-client/src/add-all.js


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


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

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

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

This reverts commit be26dd7.

This reverts commit be26dd7. The build was failing.

achingbrain · 2020-09-03T10:05:57Z

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 · 2020-09-03T10:52:25Z

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 · 2020-09-03T11:17:27Z

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?

TypeScript support for `ipfs` and `ipfs-http-client` Refs: #2945 Refs: #1166 Co-authored-by: Irakli Gozalishvili <[email protected]> Co-authored-by: Alex Potsides <[email protected]>

This reverts commit be26dd7. The build was failing.

Xmader added 9 commits August 20, 2020 18:00

docs: TypeScript support

6fccc0b

docs: add JSDoc

ac3ee3f

refactor: TypeScript support

085a5cd

docs: add JSDoc

46c7316

docs: add JSDoc

27d618e

style: fix linting

9bf714b

style: fix linting

9e1a6e4

refactor: TypeScript support

5fd264c

refactor: TypeScript support

e0f40d1

achingbrain requested a review from Gozala August 24, 2020 11:08

Gozala suggested changes Aug 25, 2020

View reviewed changes

Xmader and others added 6 commits August 24, 2020 22:06

docs: fix typo

bef93e2

docs: fix FileContent type

df5f618

fix: ... is not a function

b56d8f7

fix: keep the original behavior

48e5251

Co-authored-by: Irakli Gozalishvili <[email protected]>

docs: fix types

040a82b

docs: add JSDoc

2885568

Gozala approved these changes Aug 31, 2020

View reviewed changes

Xmader added 4 commits August 31, 2020 23:11

docs: TypeScript support

14f7dbb

refactor: reusable type definitions for multiple packages

09d0bbb

refactor: reusable type definitions for multiple packages

f62cef7

refactor: TypeScript support

2442d96

achingbrain approved these changes Sep 2, 2020

View reviewed changes

achingbrain and others added 2 commits September 2, 2020 16:59

Merge branch 'master' into jsdoc

0d4029c

refactor: TypeScript support

e9342d7

achingbrain changed the title ~~TypeScript support~~ feat: add typeScript support Sep 3, 2020

achingbrain merged commit be26dd7 into ipfs:master Sep 3, 2020

achingbrain added a commit that referenced this pull request Sep 3, 2020

Revert "feat: add typeScript support (#3236)"

e871d6e

This reverts commit be26dd7.

achingbrain added a commit that referenced this pull request Sep 3, 2020

chore: revert "feat: add typeScript support (#3236)" (#3264)

9efe907

This reverts commit be26dd7. The build was failing.

Xmader mentioned this pull request Sep 3, 2020

feat: add typescript support #3267

Merged

snyk-bot mentioned this pull request Jan 10, 2021

[Snyk] Security upgrade ipfs from 0.25.4 to 0.50.0 rtpro/js-ipfs#32

Open

snyk-bot mentioned this pull request Jan 17, 2021

[Snyk] Security upgrade ipfs from 0.25.4 to 0.50.0 rtpro/js-ipfs#33

Open

snyk-bot mentioned this pull request Apr 27, 2021

[Snyk] Security upgrade ipfs from 0.25.4 to 0.50.0 rtpro/js-ipfs#38

Open

snyk-bot mentioned this pull request May 5, 2021

[Snyk] Security upgrade ipfs from 0.25.4 to 0.50.0 rtpro/js-ipfs#40

Open

SgtPooki referenced this pull request in ipfs/js-kubo-rpc-client Aug 18, 2022

chore: revert "feat: add typeScript support (#3236)" (#3264)

4eddfed

This reverts commit be26dd7. The build was failing.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add typeScript support #3236

feat: add typeScript support #3236

Xmader commented Aug 21, 2020 •

edited

Loading

Gozala left a comment

Gozala Aug 24, 2020

Xmader Aug 25, 2020

Gozala Aug 24, 2020

Gozala Aug 24, 2020

achingbrain Aug 25, 2020

Gozala Aug 24, 2020

Gozala Aug 24, 2020

Xmader Aug 25, 2020

Gozala Aug 31, 2020

Gozala Aug 24, 2020

Gozala Aug 24, 2020

Gozala Aug 24, 2020

Xmader Aug 25, 2020 •

edited

Loading

Gozala left a comment

achingbrain left a comment •

edited

Loading

achingbrain Sep 2, 2020

Xmader Sep 2, 2020

Xmader Sep 2, 2020 •

edited

Loading

achingbrain commented Sep 3, 2020 •

edited

Loading

Xmader commented Sep 3, 2020 •

edited

Loading

achingbrain commented Sep 3, 2020

feat: add typeScript support #3236

feat: add typeScript support #3236

Conversation

Xmader commented Aug 21, 2020 • edited Loading

Gozala 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

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

Xmader Aug 25, 2020 • edited Loading

Choose a reason for hiding this comment

Gozala left a comment

Choose a reason for hiding this comment

achingbrain left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xmader Sep 2, 2020 • edited Loading

Choose a reason for hiding this comment

achingbrain commented Sep 3, 2020 • edited Loading

Xmader commented Sep 3, 2020 • edited Loading

achingbrain commented Sep 3, 2020

Xmader commented Aug 21, 2020 •

edited

Loading

Xmader Aug 25, 2020 •

edited

Loading

achingbrain left a comment •

edited

Loading

Xmader Sep 2, 2020 •

edited

Loading

achingbrain commented Sep 3, 2020 •

edited

Loading

Xmader commented Sep 3, 2020 •

edited

Loading