ts-belt icon indicating copy to clipboard operation
ts-belt copied to clipboard

Questions about common FP functions

Open anthony-khong opened this issue 2 years ago • 8 comments

Hi @mobily, the library looks really interesting! I'm quite interested in trying it out for future projects. I just have a couple of questions about certain functions, and I hope this is the right place to ask.

  1. How would you do Clojure's assoc and dissoc with ts-belt? Would it have to be D.merge(m, {k: v}) and D.rejectWithKey(m, (_, key) => key == k)?
  2. I noticed that D.xxxWithKey's signature is (v, k) instead of the usual (or rather what I would expect to be) (k, v). Is that just a matter of personal preference or rather a convention from another library/language that I'm not familiar with?
  3. On Remeda, the doc says "Due to the lazy evaluation, it’s also unclear how to use some utility functions within the pipeline." Does this mean that ts-belt does eager evaluation? So something like, say, pipe(m, D.toPairs, A.take(2)) would iterate through all the key-value pairs first before taking the first two elements?

Also, something like Remeda's mapping doc would be very helpful to newcomers! I'm happy to chip in if you think it'd be useful.

anthony-khong avatar Dec 09 '21 16:12 anthony-khong

hey @anthony-khong 👋 thanks for your questions!

  1. that sounds like proper solutions for both, but first I need to look into it
  2. actually, you're right! it definitely should be (key, value), don't you mind creating a new PR?
  3. that's right, it does the eager evaluation, it iterates through all pairs and then takes the first two elements, and frankly speaking, I don't feel that the lazy evaluation is needed here, and it's not on the roadmap at this point (although, I'm open to suggestions)

Also, something like Remeda's mapping doc would be very helpful to newcomers! I'm happy to chip in if you think it'd be useful.

I'd be more than grateful if you could create something like this! ❤️

mobily avatar Dec 09 '21 18:12 mobily

ad 2. it's been fixed and published in v3.1.1 🚀

mobily avatar Dec 09 '21 23:12 mobily

Hi @mobily, thank you very much for the response! And thank you for the patch to address (2)!

I'd be more than grateful if you could create something like this! ❤️

Absolutely, I've started looking into this. I realised though a lot of the functions I would love to use are not yet implemented, so I'm thinking of working on that first. If you're open to suggestions, would you accept contributions for Clojure's equivalent of assoc, assoc-in, dissoc, get-in, merge-with, select-keys, update and update-in? Of course, the function names can be adapted to the current function names (e.g. if we already have D.prop, it may make sense to have D.propIn instead of D.getIn).

If you agree to some of these proposals, I'd just like to make sure what it takes to implement these functions. I would have to implement the functions in src/Dict/Dict.res, declare the types in src/Dict/Dict.ts and write the tests in__tests__/Dict/xxx.test.ts, right? Am I missing any steps?

anthony-khong avatar Dec 11 '21 09:12 anthony-khong

if we already have D.prop, it may make sense to have D.propIn instead of D.getIn

that’s actually a good point in terms of the naming convention, I took the prop name directly from ramda, however, in ReScript there are get (which returns Option) and getUnsafe and I would like to stick with this nomenclature, therefore, I've already deprecated prop and added get and getUnsafe, the changes will be released in the next version

the function names can be adapted to the current function names

the function names you posted look good to me. I'm only wondering if assoc and dissoc should be named set and deleteKey accordingly (same as can be found in ReScript: https://rescript-lang.org/docs/manual/latest/api/js/dict#set), but I don't have a strong opinion on this (we can create aliases for them eventually)

I would have to implement the functions in src/Dict/Dict.res, declare the types in src/Dict/Dict.ts and write the tests in__tests__/Dict/xxx.test.ts, right?

yes, the only downside is that you need to rebuild the lib each time when you made changes (I need to rethink how to make this process more developer-friendly)

mobily avatar Dec 11 '21 16:12 mobily

the function names you posted look good to me. I'm only wondering if assoc and dissoc should be named set and deleteKey accordingly

I'm happy to change this on my latest PR (#8). Having a set of coherent function names is quite nice.

yes, the only downside is that you need to rebuild the lib each time when you made changes (I need to rethink how to make this process more developer-friendly)

The experience is okay for now. I can think of a couple of things that may be improved on top of my head:

  • Whenever I changed a .res file, I would re-run yarn build. This takes around 1 minute on my machine, which is quite long to get feedback. Perhaps we could have a minimal-build-and-test step where we can skip build steps that are not needed to run the test (e.g. doc and flow types generation)? Another thing would be to do incremental builds, but I'm not sure if that's possible.
  • If I change a .test.ts file, I would re-run yarn test. This takes around 16 seconds on my machine. I think having another script to just handpick the test would be quite nice.

I'm curious to know what you actually do during development!

anthony-khong avatar Dec 12 '21 02:12 anthony-khong

👋 hey again @anthony-khong!

I'm curious to know what you actually do during development!

I did some changes to improve this process, so basically, for development purposes, you can use the following commands:

yarn build dev -n Dict -t set

⬆️ this command will build a single module (in this case Dict) and run set tests

yarn test run -f set -n Dict

⬆️ this command will be useful if you change a single test file (in this case Dict/set.test.ts)

mobily avatar Dec 14 '21 18:12 mobily

Hi @mobily, I'd just like to get your thoughts on something. I'm thinking about implementing setIn, getIn and updateIn. In particular, getIn(obj, ['a', 'b', 'c']) would be equivalent to obj.a.b.c. I believe the legit way of doing the types here is to go the lenses way, as discussed here. However, looking at how you did pipe.ts, perhaps something like this can work?

export declare function getIn<T, K0 extends keyof T>(keys: [K0]): (dict: T) => T[K0];
export declare function getIn<T, K0 extends keyof T, K1 extends keyof T[K0]>(
  key: [K0, K1]
): (dict: T) => T[K0][K1];
export declare function getIn<
  T,
  K0 extends keyof T,
  K1 extends keyof T[K0],
  K2 extends keyof T[K0][K1]
>(key: [K0, K1, K2]): (dict: T) => T[K0][K1][K2];
export declare function getIn<
  T,
  K0 extends keyof T,
  K1 extends keyof T[K0],
  K2 extends keyof T[K0][K1],
  K3 extends keyof T[K0][K1][K2]
>(key: [K0, K1, K2, K3]): (dict: T) => T[K0][K1][K2][K3];

Questions are:

  • Do you know of a better way of doing this? It doesn't look so bad with getIn but the return types of setIn and updateIn can be a bit messy.
  • If we go with this, having multiple declarations here do not exactly fit in with declaring the consts in Dict.ts. Is there a workaround?
  • Is it worth the hassle/mess? getIn doesn't really add a lot of value in terms of expressiveness, but setIn and updateIn can be quite nice to have!

anthony-khong avatar Dec 20 '21 15:12 anthony-khong

Do you know of a better way of doing this?

this is related to #25, I will let you know once I figure it out

mobily avatar Feb 17 '22 08:02 mobily

closing, updateIn/setIn/getIn functions have been added to my TODO list, but they have a minor priority at the moment :)

mobily avatar Aug 27 '22 18:08 mobily