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

lodash.get returns any? #3771

Open
sospedra opened this issue Mar 11, 2020 · 8 comments
Open

lodash.get returns any? #3771

sospedra opened this issue Mar 11, 2020 · 8 comments

Comments

@sospedra
Copy link

sospedra commented Mar 11, 2020

This is a question and maybe a suggestion

As far as I know, get returning any is basically useless. And all uses of any discouraged by flow maintainers.

Using any is completely unsafe, and should be avoided whenever possible.

Why not use $ElementType to - at least - validate the possible types?

straight from flow docs

// @flow
function getProp<O: {+[string]: mixed}, P: $Keys<O>>(o: O, p: P): $ElementType<O, P> {
  return o[p];
}

(getProp({a: 42}, 'a'): number); // OK
(getProp({a: 42}, 'a'): string); // Error: number is not a string
(getProp({a: 42}, 'b'); // Error: `b` does not exist

I'd love to have the power of TS here but as far as I'm concerned we cannot infer as well as they do.

I'm using this currently

   get<+O: { +[string]: mixed }, S: $Keys<O>, E: mixed>(
      object: O,
      path: S,
      defaultValue?: ?E
    ): $ElementType<O, S> | E;
    get<
      +O: { +[string]: mixed },
      S1: $Keys<O>,
      S2: $Keys<$ElementType<O, S1>>,
      E: mixed
    >(
      object: O,
      path: [S1, S2],
      defaultValue?: ?E
    ): $ElementType<$ElementType<O, S1>, S2> | E;

Works only for get with object and path as string or 2-length arrays. But it's only mather to overload this method to support the array in object and N-length paths.

The question: What am I missing? 🤔

@jedwards1211
Copy link
Contributor

jedwards1211 commented Mar 11, 2020

You have to have an any or mixed fallback for string paths like 'a[0].c' that are not keys of the object. Note however that if your object has { 'a[0].b': 5 }, Lodash will actually return that root property instead of assuming the string is a path.

@jedwards1211
Copy link
Contributor

And of course you need an any or mixed fallback for array paths longer than your maximum overload.

@TrySound
Copy link
Contributor

Easier to use optional chaining which is supported by flow very well.

@jedwards1211
Copy link
Contributor

jedwards1211 commented Mar 11, 2020

I have a function that hunts for the path of a given type in a GraphQL result and returns that path as an array, which I can then use with get, so it still has marginal utility for cases like this that optional chaining can't be used for. Though theoretically I could have my function return a getter instead of a path...

@jedwards1211
Copy link
Contributor

jedwards1211 commented Mar 11, 2020

@sospedra I realized as well, if you pass a path of type string[] (i.e. flow has no idea what the actual element values are) the overloads can't infer anything about the return type.

As @TrySound points out, optional chaining works much better for this and as it becomes more widespread people will stop using get with hardcoded path arrays like ['foo', 'bar']...the only remaining use case will be dynamically built paths that Flow/TS can't infer anything about.

@sospedra
Copy link
Author

@sospedra I realized as well, if you pass a path of type string[] (i.e. flow has no idea what the actual element values are) the overloads can't infer anything about the return type.

The overload I posted works perfectly:

get<
      +O: { +[string]: mixed },
      S1: $Keys<O>,
      S2: $Keys<$ElementType<O, S1>>,
      E: mixed
    >(
      object: O,
      path: [S1, S2],
      defaultValue?: ?E
    ): $ElementType<$ElementType<O, S1>, S2> | E;

And infers properly everything. The only limitation from flow (and TS) is this.dot.syntax.
But everything else is possible.

@sospedra
Copy link
Author

This is what I have currently.
It's not properly done just some experimentation on the optionals, multiple overloads, object/array input, etc.

get<+O: { +[string]: mixed }, S: $Keys<O>>(
      object: O,
      path: S
    ): $ElementType<O, S>;
    get<+O: { +[string]: mixed }, S: $Keys<O>, E: $NonMaybeType<mixed>>(
      object: O,
      path: S,
      defaultValue?: E
    ): $NonMaybeType<$ElementType<O, S> | E>;
    get<
      +O: { +[string]: mixed },
      S1: $Keys<O>,
      S2: $Keys<$ElementType<O, S1>>,
      E: mixed
    >(
      object: O,
      path: [S1, S2],
      defaultValue?: ?E
    ): $ElementType<$ElementType<O, S1>, S2> | E;
    get<
      +O: { +[string]: mixed },
      S1: $Keys<O>,
      S2: $Keys<$ElementType<O, S1>>,
      S3: $Keys<$ElementType<$ElementType<O, S1>, S2>>,
      E: mixed
    >(
      object: O,
      path: [S1, S2, S3],
      defaultValue?: ?E
    ): $ElementType<$ElementType<$ElementType<O, S1>, S2>, S3> | E;
    get<A: $ReadOnlyArray<mixed> | RegExp$matchResult | null, I: number>(
      object: A,
      path: I
    ): mixed;
    get<O: void | null, E: $NonMaybeType<mixed>>(
      object: O,
      path: mixed,
      defaultValue: E
    ): E;

@jedwards1211
Copy link
Contributor

@sospedra what I'm saying is it will infer get(obj, ['foo', 'bar']) correctly, because ['foo', 'bar'] is a literal type that is much narrower than string[]. But when all it knows is that path is string[], as in the following example, surely there's no way it can pick the correct override.

const path: string[] = []
path.push('foo')
path.push('bar')

get(obj, path)

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