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

Add object.prop function #1454

Closed
wants to merge 3 commits into from

Conversation

cdimitroulas
Copy link
Contributor

New feature for object module:

  • added prop function

Reasoning: Small and handy utility function which is also something that many folks in the JS ecosystem are used to and learn when getting started with functional programming. Usually they use something like get from lodash or they see the prop function from the Mostly Adequate Guide to FP (https://mostly-adequate.gitbook.io/mostly-adequate-guide/appendix_c#prop).

Question: Do you prefer to add overloads so that we can have both curried/uncurried versions of this? (e.g. prop('myKey', myObject) or prop('myKey')(myObject))

@gcanti
Copy link
Owner

gcanti commented Mar 29, 2021

Thanks @cdimitroulas, two questions:

  • what's the purpose of Rest?
  • is there a way to improve the DX? In the following two use cases typescript doesn't provide any suggestion:
import { flow, pipe } from 'fp-ts/function'

export declare const prop: <Path extends string>(path: Path) => <Value, Rest>(obj: Record<Path, Value> & Rest) => Value

interface Person {
  readonly name: string
  readonly age: number
}

declare const p: Person

declare const f: () => Person

// use case 1
const g = flow(f, prop('')) // <= no suggestion, it would be nice to get "name" and "age" as suggestions
// use case 3
export const name = pipe(p, prop('')) // <= idem

@cdimitroulas
Copy link
Contributor Author

cdimitroulas commented Mar 29, 2021

The Rest property is needed otherwise in some cases the compiler won't let you pass an object with additional properties.

For example, if we pass the object directly instead of explicitly defining it as the Person interface before I get this type error if I remove the Rest from the type signature:

prop('name')({ name: "hello", age: 62 })
// Argument of type '{ name: string; age: number; }' is not assignable to parameter of type
// 'Record<"name", string>'.
// Object literal may only specify known properties, and 'age' does not exist in type
// 'Record<"name", string>'.

I'll play around with it to see if we can get autocompletion for the key argument to improve the DX, though I'm not entirely sure how to achieve it

@cdimitroulas
Copy link
Contributor Author

I tried a few variations of the type signature to see whether I could improve the DX and get suggestions for the key to be passed to prop but I was unsuccessful. If you have any ideas on how to achieve this type of thing I would be happy to try again!

@thewilkybarkid
Copy link
Contributor

Ignoring the uncurried side for the minute:

const prop = <Obj, Path extends keyof Obj>(path: Path) => (obj: Obj): Obj[Path] => obj[path];

seems close, as autocompletion on the path exists. It does mean having to duplicate the property name when not actually passing in an object, unfortunately:

const getStatusText = prop<AxiosResponse, 'statusText'>('statusText');

@cdimitroulas
Copy link
Contributor Author

cdimitroulas commented Mar 31, 2021

Thanks for looking into it @thewilkybarkid

It seems like we need to make a tradeoff here. Your solution helps to autocomplete the path (though when I tried it, it doesn't list all the possible keys for me when I open the quotes, but it does autocomplete when I start typing e.g. when I type "n" it autocompletes "name" for me). On the other hand it makes it more awkward to reuse the partially applied function for different objects because the Obj generic is fixed after the first argument (the key) is passed to prop.

For example:

const prop = <Obj, Path extends keyof Obj>(path: Path) => (obj: Obj): Obj[Path] => obj[path];

interface Person {
  readonly name: string
  readonly age: number
}

const getName = prop<Person, "name">("name")

// This no longer works because it requires me to pass in a `Person`
getName({ name: "hello" })

@samhh
Copy link
Contributor

samhh commented Mar 31, 2021

Until the type system improves might we best solve this with two different functions?:

// not checked yet, I'm saying accept anything with a 'k' property
prop('k')

// checked immediately against the provided generic
propFrom<T>()('k')

@thewilkybarkid
Copy link
Contributor

Thanks for looking into it @thewilkybarkid

It seems like we need to make a tradeoff here. Your solution helps to autocomplete the path (though when I tried it, it doesn't list all the possible keys for me when I open the quotes, but it does autocomplete when I start typing e.g. when I type "n" it autocompletes "name" for me). On the other hand it makes it more awkward to reuse the partially applied function for different objects because the Obj generic is fixed after the first argument (the key) is passed to prop.

You could make the two object types separate:

const prop = <Obj, Path extends keyof Obj>(path: Path) => <RealObj extends Record<Path, unknown>>(obj: RealObj): RealObj[Path] => obj[path];

This means that the type of the property can be different. If not desired, that type can be reused:

const prop = <Obj, Path extends keyof Obj>(path: Path) => <Value extends Obj[Path], RealObj extends Record<Path, Value>>(obj: RealObj): Value => obj[path];

But not sure if you should be able to pass in a partial object anyhow. (Sounds like the original generic object should be smaller.)

@cdimitroulas
Copy link
Contributor Author

Neither of those two implementations work for me:

Screenshot from 2021-03-31 17-32-54

@anthonyjoeseph
Copy link
Contributor

anthonyjoeseph commented Mar 31, 2021

it makes it more awkward to reuse the partially applied function for different objects

I'm not sure this is possible in typescript (source):

const a: <A extends Record<'a', unknown>>(a: A) => A['a']  = prop('a')

The current implementation yields this, which seems less useful:

const a: (a: Record<'a', unknown>) => unknown  = prop('a')

Could something like this work?

(Edit: quote added)

@thewilkybarkid
Copy link
Contributor

thewilkybarkid commented Mar 31, 2021

Neither of those two implementations work for me:

Sorry, meant need to combine with #1454 (comment). Think this covers all the cases:

declare function prop<Obj, Path extends keyof Obj>(path: Path): {
  <RealObj extends Record<Path, unknown>>(obj: RealObj): RealObj[Path]
  (obj: Obj): Obj[Path]
}
declare function prop<Path extends string>(path: Path): <Obj extends Record<Path, unknown>>(obj: Obj) => Obj[Path]

declare function propW<Obj, Path extends keyof Obj>(path: Path): {
  <RealObj extends Record<Path, unknown>>(obj: RealObj): RealObj[Path]
}
declare function propW<Path extends string>(path: Path): <Obj extends Record<Path, unknown>>(obj: Obj) => Obj[Path]

Try this playground

Edit: just noticed the overloading loses the name autocompletion...

Edit 2: This removes overloading, restoring autocompletion but means having to add explicit generics for the reusable functions.

@cdimitroulas
Copy link
Contributor Author

Could something like this work?

@anthonyjoeseph I personally find it weird that it infers as unknown when you write something like prop('a') but maybe that's just me :D

@thewilkybarkid what's propW supposed to do beyond prop? In your playground link in Edit 2 it seems identical to prop. Thanks for all the effort to find the best iteration of this function btw!

@thewilkybarkid
Copy link
Contributor

thewilkybarkid commented Mar 31, 2021

@thewilkybarkid what's propW supposed to do beyond prop? In your playground link in Edit 2 it seems identical to prop. Thanks for all the effort to find the best iteration of this function btw!

Here's the last playground for tonight. Managed to keep autocompletion working, but allow for either no generics or only an object type (so no need to also pass in the property name too). Since it works without any generics at all, propW is now redundant (was meant to allow using the property names of some type but allowing a different return type; came from #1454 (comment) but definitely doesn't seem needed now).

@cdimitroulas
Copy link
Contributor Author

Nice! The only issue I see with that is that we default to any for the Path generic, which means that we lose type safety.

For example, this compiles even though the key does not exist in Person:

const getPersonName = prop<Person>('banana')

@anthonyjoeseph
Copy link
Contributor

anthonyjoeseph commented Mar 31, 2021

OK I was definitely wrong about the partial application thing - my mistake. Taking inspiration from @thewilkybarkid 's excellent work, I think I found a solution that doesn't need a propW doesn't default to any (source) It allows for both autocomplete & partial application. However it's a bit ugly, I'm not sure how important that is

@cdimitroulas
Copy link
Contributor Author

Nice one, this looks quite good in terms of how well it works with the various use-cases.

It's a bit scary that we need to use two assertions to any ((obj as any)[path as any]) 😱 ) but maybe it's fine as I couldn't find any case where this actually impacted on type-safety with a few attempts that I made.

@anthonyjoeseph
Copy link
Contributor

That can be solved with a couple less scary assertions

@cdimitroulas
Copy link
Contributor Author

Perfect! This works great. I'll update the PR with this new implementation. Are we happy to drop the uncurried overload? Personally I'm ok with it

@thewilkybarkid
Copy link
Contributor

I've just had a play and looks like the best option. 👍

Couldn't simplify the type any further.

@cdimitroulas
Copy link
Contributor Author

@gcanti for some reason I got a bunch of unrelated docs changes after I rebased my branch and I reran npm run test

@thewilkybarkid
Copy link
Contributor

Turned my playground into a dtslint file (cdimitroulas#1).

@gcanti
Copy link
Owner

gcanti commented Apr 1, 2021

@cdimitroulas ah sorry, I'm not generating the docs on every commit as it slowed me down.

I was thinking about the usefulness of a prop function, what are the use cases?

This use case is not compelling

interface Person {
  readonly name: string
  readonly age: number
}

declare const p: Person

pipe(p, prop('name'))

as you can just project with the same effort (and standard autocompletion)

pipe(p, (_) => _.name)

If I want a generic "accept anything with a 'name' property", I would expect a polymorphic function as a result:

// const getName: <A>(obj: Record<"name", A>) => A
const getName = prop('name')

Another use case that would be nice is to make contramaps easier to define:

import * as S from 'fp-ts/string'
import * as Ord from 'fp-ts/Ord'
import * as Eq from 'fp-ts/Eq'

interface Person {
  readonly name: string
  readonly age: number
}

declare const p: Person

const eq = pipe(
  S.Eq,
  Eq.contramap((p: Person) => p.name) // <= awkward
)

// const myEq: Eq.Eq<Record<"name", string>>
const myEq = pipe(S.Eq, Eq.contramap(prop('name'))) // nice

// const myOrd: Ord.Ord<Record<"name", string>>
const myOrd = pipe(S.Ord, Ord.contramap(prop('name'))) // nice

Other use cases?

@cdimitroulas
Copy link
Contributor Author

This use case is not compelling {...} as you can just project with the same effort (and standard autocompletion)
pipe(p, (_) => _.name)

I can understand your argument, but I think many in the ecosystem find this to be slightly awkward as well, and would prefer to do it with a function like prop. Especially those who are fans of point-free style.

The contramap use-case is nice 👍

@anthonyjoeseph
Copy link
Contributor

I would expect a polymorphic function as a result

That's the current behavior - would you like to remove the autocomplete functionality to simplify it?

@cdimitroulas
Copy link
Contributor Author

cdimitroulas commented Apr 19, 2021

I tested out the contramap use-case you mentioned @gcanti and it works only if we add the explicit type annotations to contramap like so:

// const myEq: Eq.Eq<Record<"name", string>>
const myEq = pipe(Eq.eqString, Eq.contramap<string, Person>(prop('name'))) // nice

// const myOrd: Ord.Ord<Record<"name", string>>
const myOrd = pipe(Ord.ordString, Ord.contramap<string, Person>(prop('name'))) // nice

So it doesn't seem like the prop function really helps to keep this terse unfortunately

@anthonyjoeseph
Copy link
Contributor

anthonyjoeseph commented Apr 19, 2021

Good catch @cdimitroulas ! I think I've solved this (source) - please forgive my any casts, I'm still working on that

Edit: updated to remove the any type

@raveclassic
Copy link
Collaborator

raveclassic commented Apr 19, 2021

Hey there, thanks for the work and for the proposal!
Here are my 2 cents: shouldn't this be a part of a 3rd-party library? This reminds me lenses a lot, monocle-ts for instance.

I'm concerned about the boundaries of fp-ts. There are some questions that I'd first like to discuss:

  • will we add an index accessor for arrays? If not, why not? We already have a "prop" accessor. If yes, then how about other helpers?
  • are we going to reimplement libraries like ramda and similar in fp-ts core? It seems that the main goal of fp-ts - emulation of HKTs in TypeScript - diverges from such proposal.

@cdimitroulas
Copy link
Contributor Author

@raveclassic there's a lot of discussion over at #1460 relating to similar struct util functions, so I think that would be a good place to bring this up as I believe your concerns would carry over to those other functions as well

@anthonyjoeseph
Copy link
Contributor

anthonyjoeseph commented Apr 19, 2021

@raveclassic very good points, definitely important stuff to consider.

I know this wasn't your point, but you made me realize that prop could support tuples as well - POC

Edit: updated to remove the any type

@samhh
Copy link
Contributor

samhh commented Apr 19, 2021

Slightly unrelated but @raveclassic I've got this document that attempts to keep track of Ramda equivalency in fp-ts[-std], it might prove useful.

@gcanti gcanti closed this Jul 26, 2021
@gcanti gcanti deleted the branch gcanti:2.11 July 26, 2021 07:35
@anthonyjoeseph
Copy link
Contributor

@cdimitroulas would you mind updating this PR to be against master? I think it got auto-closed along with the 2.11 branch

@cdimitroulas
Copy link
Contributor Author

Done @anthonyjoeseph

@anthonyjoeseph
Copy link
Contributor

anthonyjoeseph commented Aug 25, 2021

I've published a library called spectacles-ts that mirrors this functionality (and is a facade for most of monocle-ts) - it'd be great to get feedback from all you smart folks on this thread!

import { get } from 'spectacles-ts'
import * as Eq from 'fp-ts/Eq'
import { Eq as StringEq } from 'fp-ts/string'

const piped: number = pipe({ a: { b: 123 } }, get('a', 'b'))
const polymorphic: number = get('a', 'b')({ a: { b: 123 } })
const inferred: Eq.Eq<{ a: { b: string } }> = pipe(
  StiringEq,
  Eq.contramap(get('a', 'b'))
)

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.

6 participants