Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix TypeScript type inference for functions passed to map #457

Merged
merged 6 commits into from
Mar 13, 2021
Merged

Conversation

Avaq
Copy link
Member

@Avaq Avaq commented Jan 7, 2021

This pull request tries to fix #455

I've started with a tsd setup to defend against regressions to before #401 and #403. Run npm run test:types to check that your typings have not introduced a regression.

For context, here's the current TypeScript typings for map:

Fluture/index.d.ts

Lines 137 to 143 in 2f12660

/** Map over the resolution value of the given Future or ConcurrentFuture. See https://github.com/fluture-js/Fluture#map */
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;

Also relevant, the type of pipe:

Fluture/index.d.ts

Lines 36 to 37 in 2f12660

/** Apply a function to this Future. See https://github.com/fluture-js/Fluture#pipe */
pipe<T>(fn: (future: FutureInstance<L, R>) => T): T

@Avaq Avaq self-assigned this Jan 7, 2021
@codecov

This comment has been minimized.

@Avaq
Copy link
Member Author

Avaq commented Jan 7, 2021

I've pushed an additional set of test assertions to check that the requirements stipulated in #455 are met. As of yet, these assertions fail and I have been unable to find a type definition that causes the assertions to pass.

@Avaq
Copy link
Member Author

Avaq commented Jan 7, 2021

Of course, if I replace the entire TS definition for map with the following, then the new assertions pass:

export function map<RA, RB>(mapper: (value: RA) => RB):
  <L>(source: FutureInstance<L, RA>) => FutureInstance<L, RB>

The definition above is functionally equivalent to the definition that was used in Fluture v11. There is no support for the ConcurrentFuture type, and so while all the other assertions now pass, the ones for the ConcurrentFuture type fail (#401).

The approach here was designed by @tjjfvi, who has been a tremendous
help on the TypeScript Discord server. Thank you!

Co-Authored-By: tjjfvi <tjjfvi@tjjfvi.com>
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
Switch to using a symbol instead of the 'input' property, to prevent
this property, which is just used for passing around type information,
from showing up in auto-completion as if it could be used at runtime.
index.d.ts Outdated Show resolved Hide resolved
@Avaq
Copy link
Member Author

Avaq commented Jan 12, 2021

Should I release this with a major version bump? I would prefer not to, but I find that when changing TS types, it usually probably breaks something for someone.

@Avaq
Copy link
Member Author

Avaq commented Jan 12, 2021

We've kind of "narrowed" the type of map here from map(f: (x: unknown) => B) to map(f: (x: A) => B) where A can be inferred from the second argument. Narrowing a type is usually a breaking change. In this case, we're narrowing the type to exactly what the user would want, though. So chances are users will have no problems. Unless a user gave an incorrectly typed function to map, in which case this change will suddenly break their build. Although in the latter case one might argue that their code was already broken, they just didn't know it.

@Avaq Avaq merged commit 55e16f5 into master Mar 13, 2021
@Avaq Avaq deleted the avaq/map-ts branch March 13, 2021 11:25
Avaq added a commit that referenced this pull request Mar 13, 2021
- #457 TypeScript type inference for the `map` function was improved.
- #457 The TypeScript type for `map` has been narrowed, and might
  now cause a compile error.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to infer types in function passed to map
2 participants