gcanti / fp-ts

Functional programming in TypeScript
https://gcanti.github.io/fp-ts/
MIT License
10.78k stars 502 forks source link

lookup types incorrect #1900

Closed levino closed 1 year ago

levino commented 1 year ago

🐛 Bug report

Current Behavior

When using lookup one gets problematic types for the return value.

Expected behavior

lookup correctly infers the type from the type of the second argument.

Reproducible example

https://codesandbox.io/s/lookup-type-problem-ts92vh

import { lookup } from "fp-ts/Record";
import assert from "assert";
import * as O from "fp-ts/Option";
import { identity, pipe } from "fp-ts/function";
import { toUpperCase } from "fp-ts/string";

const user = {
  name: "Sam",
  age: 77
};

assert.strictEqual(
  pipe(
    user,
    lookup("name"), // Should not have a TS-error here.
    O.map(toUpperCase),
    O.match(() => "error", identity)
  ),
  "SAM"
);

Suggested solution(s)

I do not know, sorry. The problem is that the second argument is considered Record<string, A>. So basically this only works for objects where all values are of the same type, which is a pretty hefty restriction imo. In this case, lookup("name") should return Option<string> and not Option<string | number>.

samhh commented 1 year ago

Very similar to #1899, see my comment in there. fp-ts-std/Struct::get should do what you want.

levino commented 1 year ago

Actually, I think it does not. I expanded the example. My datatype has an optional property, so the Option container is a great return type.

https://codesandbox.io/s/lookup-type-problem-expanded-p6fvgt?file=/src/index.ts

import { lookup } from "fp-ts/Record";
import assert from "assert";
import * as O from "fp-ts/Option";
import { identity, pipe } from "fp-ts/function";
import { toUpperCase } from "fp-ts/string";

const users: { name?: string; age: number }[] = [
  {
    name: "Sam",
    age: 77
  },
  {
    age: 21
  }
];

assert.strictEqual(
  pipe(
    users[0],
    lookup("name"), // Should not have a TS-error here.
    O.map(toUpperCase),
    O.match(() => "error", identity)
  ),
  "SAM"
);

assert.strictEqual(
  pipe(
    users[1],
    lookup("name"), // Should not have a TS-error here.
    O.map(toUpperCase),
    O.match(() => "error", identity)
  ),
  "error"
);

Maybe something like safeGet would be helpful?

samhh commented 1 year ago

Maybe something like safeGet would be helpful?

There are various ergonomic challenges with optional properties such that I try to guide folks towards using Option directly instead e.g. using io-ts-types' optionFromNullable, which'd naturally work with get.

Else you can use this:

const getOptional =
  <K extends string>(k: K) =>
  <A>(x: Partial<Record<K, A>>): O.Option<A> =>
    // Beware `O.fromUndefined` should `A` itself be `undefined` as a value rather than missing.
    k in x ? O.some(x[k]!) : O.none
levino commented 1 year ago

Thank you @samhh for taking the time to answer in such detail. The care to detail, the quality of response and the supportiveness in the FP-community always baffles me again. I will dig into io-ts to see how I can make my optional values options for values.

Closing the issue as "wontfix": As mentioned above by @samhh, a Record is thought of as a "lookup table" or a "dictionary" and expected to have values of a uniform type. In my case, I was using a Struct.