This repository was archived by the owner on Feb 12, 2024. It is now read-only.
chore: move withTimeoutOption to core-utils#3407
Merged
achingbrain merged 2 commits intomasterfrom Nov 18, 2020
Merged
Conversation
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
Gozala
reviewed
Nov 26, 2020
| /** | ||
| * @template {any[]} ARGS | ||
| * @template {Promise<any> | AsyncIterable<any>} R - The return type of `fn` | ||
| * @param {Fn<ARGS, R>} fn |
Contributor
There was a problem hiding this comment.
@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.
Contributor
There was a problem hiding this comment.
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pulls non-grpc changes out of #3403 to ease the continued merging of
master into that branch.