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

Real world usage of omit is now awkward #108

Open
robations opened this issue Mar 5, 2024 · 6 comments
Open

Real world usage of omit is now awkward #108

robations opened this issue Mar 5, 2024 · 6 comments

Comments

@robations
Copy link

I think the types of omit have been strengthened recently as (since minor upgrades to v0.29) I have a number of type errors that are flagged, especially for omit.

If you have a function signature, you may omit certain object keys in the sense, "I'm only interested in a subtype". This allows, say, unit tests to be more minimal and aligns with the principle of least knowledge. But in usage, the omitted keys may be present and you may want to acknowledge this in the implementation.

I'm not sure this is the most minimal example, but I hope shows what I mean:

import R from "ramda";

export interface Params {
    foo: string;
    bar?: string;
}

export function fn1(f: typeof fn2) {
    return (params: Omit<Params, "bar">) => {
        // imagine this omission is an important use case
        return f(R.omit(["bar"] as const, params)); // TS2322
    };
}

function fn2({ foo, bar }: Params): boolean {
    return false;
}

const a: Params = {foo: "foo", bar: "bar"};

fn1(fn2)(a);

See also TS Playground link

In other words, I would expect seemingly unrelated keys not to be a type error. I can probably work around this by creating my own omit implementation, but this goes against the spirit of ramda being a useful multi-tool library when I can't actually use it in a multitude of cases.

I can already hear gnashing of teeth, because probably someone has worked hard to add the new types. Of course, I can see the opposite argument that the strong typing will flag up potential typos.

@robations
Copy link
Author

There is also a confusing type error generated which suggests that both foo and bar are missing from the f() argument. I can see how this might happen but it may confuse anyone trying to debug the omit call. "params clearly has foo, wtf?"

@Harris-Miller
Copy link
Collaborator

Harris-Miller commented Mar 6, 2024

Reviewing your TS Playground, the types for Omit<> seem to be working as intended

Let's examine it:

import R from "ramda";

export interface Params {
    foo: string;
    bar: string;
}

export function fn1(f: typeof fn2) {
    return (params: Omit<Params, "bar">) => {
        return f(R.omit(["bar"] as const, params));
    };
}

function fn2({ foo, bar }: Params): boolean {
    return false;
}

const a: Params = {foo: "foo", bar: "bar"};

fn1(fn2)(a);

The error you're getting is in fn1

export function fn1(f: typeof fn2) {
    return (params: Omit<Params, "bar">) => {
        return f(R.omit(["bar"] as const, params)); // Error: see below
    };
}

The error:

Argument of type 'Omit<Omit<Params, "bar">, "foo">' is not assignable to parameter of type 'Params'.
  Type 'Omit<Omit<Params, "bar">, "foo">' is missing the following properties from type 'Params': foo, bar`

Honestly, this is fucking weird. Why is complaining about Omit<~, "foo">? "foo" is not in the type or the const array. And that it's missing prop foo at all? I don't think that issue is Ramda, I think the problem is that because you have params: Omit<Params, "bar"> typed that way, trying to then omit(["bar"] as const, params_ from a variable that doesn't have prop bar is confusing typescript

This segways me into part 2 here. The typings for fn1 don't match the intention of the function body

params: Omit<Params, "bar"> is saying "params is typeof Params, with prop bar removed. But in the body you're R.omit(["bar"] as const, params). So wouldn't it just be params: Params?

export function fn1(f: typeof fn2) {
    return (params: Params) => {
        return f(R.omit(["bar"] as const, params)); // still Error, but better
    };
}

The error:

Argument of type 'Omit<Params, "bar">' is not assignable to parameter of type 'Params'.
  Property 'bar' is missing in type 'Omit<Params, "bar">' but required in type 'Params'.

Now we get the error I'm expecting, with no mention of foo.

Why are we getting this error? That's simple. f is typeof f2. And f2 is defined as

function fn2({ foo, bar }: Params): boolean {
    return false;
}

fn2 is expecting an argument of type Params, but fn1 is passing it a type Omit<Params, "bar">. This is type safety YOU WANT! It's type guarding exactly as intended, and is no different from if you did this:

fn2({ foo: '' });
// ^^ Error:
Argument of type '{ foo: string; }' is not assignable to parameter of type 'Params'.
  Property 'bar' is missing in type '{ foo: string; }' but required in type 'Params'.

Without this error, you may get a runtime error when trying to access a method on bar, or anything else with it, because it's undefined and not string

You mentioned unit tests. If your goal is to test how the runtime behavior deals with missing props, then casting the type in your test is the appropriate solution to get around this type guard error

export function fn1(f: typeof fn2) {
    return (params: Params) => {
       // test `f` is it unexpectedly receives `Omit<Params, 'bar'>` instead of `Params`
        return f(R.omit(["bar"] as const, params) as Params);
    };
}

If at runtime the prop bar is generally expected to be possibly undefined, then the typeof Params should reflect that

type Params {
  foo: string;
  bar?: string // now optional
};

I'm making a lot of assumption based solely off that simple playground example, as well as how you mentioned Unit tests. So I'm hoping I'm not way off base with my assertions against your issues. Please let me know if I am and we can work on making some better examples in the TS Playground to see where the changes to the types for omit break down for you

@fuadsaud
Copy link

For me, the argument on why omit should accept any key is pretty simple: it should align with TypeScript's Omit. Omit doesn't care about keys, why should Ramda's types? In this case, I think R.omit should mirror TypeScript's own Omit:

type Foo = {x: string, y: string}
type WithoutZ = Omit<Foo, 'z'> // this works just fine

const foo = {x: "this is X", y: "this is Y"}

R.omit('z', {foo: 'value'}) // this should work too and the return value should be equivalent to WithoutZ

TypeScript types are loose, so the objects that match a type with keys x, and y could also contain key z. I think it's fair that some code might want to make sure that a certain key is absent from an object:

type Foo = {x: string, y: string}

const fooInstance = // data fetched from somewhere else, parsed using zod, manipulated along a tranformation pipeline, you name it

function sendFooToExternalService(foo: Foo) {
  send(R.omit('someKeyThatMightHaveBeenAddedAlongTheWay', foo))
}

The argument for not accepting unknown keys is to avoid typos. Maybe I really meant to omit y in the above code, but made a typo and omitted z instead. It's a tradeoff and I understand that different people may take different sides. My personal take is:

  • I think ramda's omit should align with TypeScript's Omit
  • I get more value from being able to manipulate data freely than from checking for typos.

@Harris-Miller
Copy link
Collaborator

The argument for not accepting unknown keys is to avoid typos. Maybe I really meant to omit y in the above code, but made a typo and omitted z instead. It's a tradeoff and I understand that different people may take different sides.

IMHO It's better to be strict and allow you to opt-out of that strictness, than remove it altogether. You can always get around this by // @ts-expect-error or by casting to Record<string, Whatever>

@Harris-Miller
Copy link
Collaborator

It did occur to me that ValueOfUnion<> is needed to correctly support all available keys of union types. I'll try and get that in soon

@Harris-Miller
Copy link
Collaborator

Also, while Omit<> doesn't care about the keys, Pick<> does

And you look at the implementation of Pick, ten look at how Omit is implemented as a function of Pick, the fact that it doesn't care about the keys is a bug. It likely should care.

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

No branches or pull requests

3 participants