Unable to infer types in function passed to map

[EricCrosson]

Hello,

I am still using fluture 11.x in my biggest projects, and have been starting to dip my toes into v13. I may be missing something, but there seems to be an annoying bug where the function I pass to map cannot infer types.

To reproduce:

tsconfig.json

{
    "include": [
        "src/**/*",
        "test/**/*"
    ],
    "exclude": [
        "node_modules",
        "dist"
    ],
    "compilerOptions": {
        "target": "es5",
        "module": "commonjs",
        "lib": ["es6"],
        "declaration": true,
        "declarationMap": true,
        "sourceMap": true,
        "outDir": "./dist",
        "downlevelIteration": true,
        "strict": true,
        "baseUrl": "./",
        "paths": {
            "*": ["src/@types/*"]
        },
        "esModuleInterop": true
    }
}

package.json

{
  "name": "repro-fluture",
  "version": "0.0.1",
  "devDependencies": {
    "@types/node": "^14.14.17",
    "ts-node": "^9.1.1",
    "typescript": "^4.1.3"
  },
  "dependencies": {
    "fluture": "^13.0.1"
  }
}

src/index.ts

import * as F from 'fluture'

F.resolve(5)
    .pipe(F.map(value => value.toString()))

This code gives the error:

src/repro-fluture-13-map-type-bug.ts:4:26 - error TS2571: Object is of type 'unknown'.

4     .pipe(F.map(value => value.toString())
                           ~~~~~

Here the type of value is unknown, so the toString() cannot be type-checked.

I find it strange that nobody has reported this, but I am able to reproduce in several of my repos, with various tsconfig settings.

[EricCrosson]

I should note that chain can infer types just fine (i.e. same behavior as fluture 11.x), so this is specific to map

[Avaq]

Hi @EricCrosson. Thanks for the report. I think it's because the second argument to map is overloaded with support for the ConcurrentFuture type besides the Future type:

Fluture/index.d.ts

Lines 138 to 143 in 2f12660

	export function map<RA, RB>(mapper: (value: RA) => RB): <T extends FutureInstance<any, RA> | ConcurrentFutureInstance<any, RA>>(source: T) =>
	T extends FutureInstance<infer L, RA> ?
	FutureInstance<L, RB> :
	T extends ConcurrentFutureInstance<infer L, RA> ?
	ConcurrentFutureInstance<L, RB> :
	never;

TypeScript cannot infer types from overloaded functions (microsoft/TypeScript#32418 (comment)).

---

In Fluture v11, this overload was incorrectly specified at the first function argument:

Fluture/index.d.ts

Lines 271 to 277 in 00959c7

	/** Map over the resolution value of the given Future. See https://github.com/fluture-js/Fluture#map */
	export function map<L, RA, RB>(mapper: (value: RA) => RB, source: FutureInstance<L, RA>): FutureInstance<L, RB>
	export function map<L, RA, RB>(mapper: (value: RA) => RB): (source: FutureInstance<L, RA>) => FutureInstance<L, RB>
	/** Map over the resolution value of the given ConcurrentFuture. See https://github.com/fluture-js/Fluture#map */
	export function map<L, RA, RB>(mapper: (value: RA) => RB, source: ConcurrentFutureInstance<L, RA>): ConcurrentFutureInstance<L, RB>
	export function map<L, RA, RB>(mapper: (value: RA) => RB): (source: ConcurrentFutureInstance<L, RA>) => ConcurrentFutureInstance<L, RB>

This means that TypeScript always resolves to the first overload that matches the input function (as opposed to the type of Functor), which is the version of map used on Future (see #400). So in v11, it worked for Future instances, but not for ConcurrentFuture instances. This was fixed over several iterations: first #401 and then #403.

---

I believe this is a case where fixing one issue introduces the other, and vice-versa. I've also had trouble with TypeScript's inference from overloaded functions when I was trying to do #438, leading me to have abandoned it for now. I would love for this problem to be solved somehow, but no luck so far. We can leave this issue open for visibility.

[Avaq]

Oh, and the reason my preference is to leave it at the current solution, and not go back to the v11 way (essentially dropping support for map on ConcurrentFuture), is because the problem caused by the current approach is easier to solve in userland, via a type annotation. In other words, it seems like the lesser of two evils.

[Avaq]

@EricCrosson As a work-around, you could create a Future-specialized map function, eg:

import * as F from 'fluture'

export function map<RA, RB>(mapper: (value: RA) => RB):
  <L>(source: F.FutureInstance<L, RA>) => F.FutureInstance<L, RB> {
  return F.map (mapper)
}

///////////////////////////////////////////////

F.resolve(5)
    .pipe(map(value => value.toString()))

[EricCrosson]

@Avaq, thank you for the quick response and suggested workaround!

I appreciate the historical context you have provided. There's a lot to read and think about, so I may take some time to digest all of this.

[Avaq]

Following some links, I found this comment:

Overloads, whenever possible, should always have a final overload that covers all of the cases in a more general way
-- microsoft/TypeScript#40413 (comment)

This makes it sound like perhaps the problematic overload can be rephrased for TypeScript to "fall back" to the "map on Future" case when it can't resolve the overload. I will investigate this option.

[EricCrosson]

Another option I have been thinking about is to create more separation between the Future and ConcurrentFuture types, so that both can support their entire feature-set "out of the box", but I like the sound of the final overload much more

[Avaq]

@EricCrosson I've started working on this, although have been unsuccessful in finding a solution. See #457.

[Avaq]

I also asked for help with this issue on TypeScript's Discord: https://discord.com/channels/508357248330760243/740274647899308052/796759112607203399

[EricCrosson]

Awesome, I didn't know there was a TypeScript discord! I'll check out your post there and the PR to see if I can help

[Avaq]

Someone on the Discord channel found a solution. I committed it to #457. Perhaps you could install npm i fluture-js/fluture#avaq/map-ts and test if this has solved your issue? Note that this may not work with ts-node due to its reliance on CJS, but you can test with tsc.

[EricCrosson]

Sure! I'm reading through the discord conversation now -- coolest back-and-forth I've seen in a while 😎

Where @tjjfvi mentioned

I've never fully grokked the other [HKT] encodings I've seen

it brought this repo to mind as it's the HKT implementation that has made the most sense to me to date, but I'm not convinced it's any simpler than his solution. I learned a ton today! Many thanks to you both 🙏

Testing fluture-js/fluture#avaq/map-ts locally

Yes sir! The example in my initial post compiles without a hitch, and my editor correctly infers that value is of type number. This ticket can be closed. Dynamite! 🧨

Consider me officially excited to upgrade my projects from fluture 11 to fluture 🚀

[tjjfvi]

it brought this repo to mind as it's the HKT implementation that has made the most sense to me to date, but I'm not convinced it's any simpler than his solution. I learned a ton today! Many thanks to you both 🙏

That traverses down the type and replaces _<0>, _<1>, etc., which thus doesn't support conditional types, which is normally half of my use case 😄 . Creating the HKTs with it is a bit simpler, however, so if you're just using HKTs for a Functor-esque purpose like this, it's probably better.