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

chore: move withTimeoutOption to core-utils #3407

Merged
merged 2 commits into from
Nov 18, 2020

Conversation

achingbrain
Copy link
Member

Pulls non-grpc changes out of #3403 to ease the continued merging of
master into that branch.

  • Moves withTimeoutOption into core-utils
  • Moves TimeoutError into core-utils
  • Adds missing ts project links
  • Adds more add-all tests to interface suite
  • Ignores unpassable tests for non-grpc or core implementations
  • Normalises mode and mtime in normalise-input function
  • Dedupes mtime normalisation between core and http client

Pulls non-grpc changes out of #3403 to ease the continued merging of
master into that branch.

- Moves withTimeoutOption into core-utils
- Moves TimeoutError into core-utils
- Adds missing ts project links
- Adds more add-all tests to interface suite
- Ignores unpassable tests for non-grpc or core implementations
- Normalises mode and mtime in normalise-input function
@achingbrain achingbrain merged commit 288a259 into master Nov 18, 2020
@achingbrain achingbrain deleted the chore/move-with-timeout-option-to-core-utils branch November 18, 2020 17:45
/**
* @template {any[]} ARGS
* @template {Promise<any> | AsyncIterable<any>} R - The return type of `fn`
* @param {Fn<ARGS, R>} fn
Copy link
Contributor

Choose a reason for hiding this comment

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

@achingbrain this seems to have introduced a regression, because Fn typedef ended up in index.js which is not imported here. Which for some reason TS still seems to kind of pick up, but it no longer behaves correctly & causing returned function to be inferred as any.

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 the whole Fn type is kind of redundant and we should probably change type annotation for withTimeoutOptions to something like this instead:

/**
 * @template {any[]} Args
 * @template {Promise<any> | AsyncIterable<any>} R - The return type of `fn`
 * @param {(...args:Args) => R} fn
 * @param {number} [optionsArgIndex]
 * @returns {(...args:Args) => R}
 */

achingbrain pushed a commit that referenced this pull request Nov 26, 2020
#3407 has introduced regression in type inference for `withTimeoutOptions` function which end up referring to `Fn<Args, R>` type which was declared in other file https://github.com/ipfs/js-ipfs/pull/3407/files#diff-722621abc3ed4edc6ab202fdf684f1607c261394b95da6b3ec79748711056f20

I think TS has this strange WTF where when it can't figure out when JS file is a module it will treat it as if everything is loaded in the same scope. I think that caused `withTimeoutOptions` not to report errors but silently fail and infer return function as `any`.

This change fixes that by removes `Fn` type all together, as it seemed like unnecessary complexity. 

Unfortunately nothing seemed to have caught this regression because we set [`noImplicitAny`](https://www.typescriptlang.org/tsconfig#noImplicitAny) to `false` given that many of our dependencies are untyped.
SgtPooki referenced this pull request in ipfs/js-kubo-rpc-client Aug 18, 2022
Pulls non-grpc changes out of #3403 to ease the continued merging of
master into that branch.

- Moves withTimeoutOption into core-utils
- Moves TimeoutError into core-utils
- Adds missing ts project links
- Adds more add-all tests to interface suite
- Ignores unpassable tests for non-grpc or core implementations
- Normalises mode and mtime in normalise-input function
- Dedupes mtime normalisation between core and http client
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