Implement native map and bind functions for RecoilState

[Shmew]

Having a correctly implemented map and bind for a type allows for very powerful abstractions and data transformations.

I have an implementation that given a recoil value it creates a selectorFamily that accepts a lifted function (via constSelector), which then fetches the dependencies, and then returns the new value.

Bind we somewhat get for free as recoil automatically flattens recoil values, so it's the same logic as map.

I'm not terribly confident that this can't somehow cause name conflicts at some point, and having a native implementation of these functions would be great.

I think this also ties in a bit with #314 as ideally the mapped value wouldn't cause a re-render when the parent changes if the resulting output didn't change (such as a simple number atom that had something like this applied: myAtom.map(x => x * 0)

[drarmstr]

@Shmew - I think providing a map() method for RecoilValue as shorthand for creating an implicit selector is an interesting idea. We toyed with the idea, but haven't explored it. Questions remain about unique key construction for dev tools and normalization of redundant mappings.

You can do something similar with the existing map() method on a Loadable, but that really isn't idiomatic and also wouldn't take advantage of normalization and suppressing updates and subscriptions down the data-flow graph for unchanged values, etc.

[Shmew]

Yeah, I think simply appending /map to the original key works well here, the only thing I've struggled with/feel uncertain about is when lifting a function up into a recoil value via the constSelector and if it can cause collisions somehow. I think it's fine, and so far everything has worked exactly as expected.

I transpile my code from F#, but I think the implementation here should be easy enough to understand:

let mapFamily (recoilValue: RecoilValue<'T,'Mode>) =
    selectorFamily {
        key (recoilValue.key + "/__map")
        get (fun (mapping: RecoilValue<'T -> 'U,ReadOnly>) getter ->
            getter.get(recoilValue) |> getter.get(mapping))
    }

let inline lift (value: 'T) = Bindings.Recoil.constSelector value

let map (mapping: 'T -> 'U) (recoilValue: RecoilValue<'T,'Mode>) =
    lift mapping
    |> mapFamily recoilValue

[drarmstr]

@Shmew - I threw #319 together to evaluate this. So far it appears nice, but not yet viable. The selectors are memoized based on the mapper function reference equality. So, using myAtom.map(x => x * 100) in a render function would cause a new selector to be implicitly created with each render, which is a leak. Even if we add cleanup of unused selectors it adds overhead. The user could memoize their mapping function, but it's not obvious from the API that they should do that.

[Shmew]

Interesting, so I don't use map inside the actual render functions as it defeats the purpose of using recoil in my opinion, since the resulting selector isn't able to be used outside of that scope. I'm not entirely sure how to best solve this as my javascript knowledge isn't the best.

Here's am example of how I ideally would use this (once #314 is implemented):

type Model =
    { Count: int
      Text: string
      SomeOtherText: string }

let model = 
    atom { 
        key "myModel"
        def {
            Count = 0
            Text = "Hello world!"
            SomeOtherText = ""
        }
    }

let count = model.map(fun m -> m.Count)
let countAddFifty = count.map(fun c -> c + 50)

let text = model.map(fun m -> m.Text)
let textCount = text.bind(fun t -> countAddFifty.map(fun c -> sprintf "%s - %i" t c))

let someOtherText = model.map(fun m -> m.SomeOtherText)

[drarmstr]

@Shmew - Yeah, I think map() would work great in that situation as a short-hand. The problem is that the API would also encourage its use in render functions, and people would unknowingly cause selector churn. Not sure how to avoid that yet..

For example, the following is convenient to write, but problematic:

function MyComponent() {
  const value = useRecoilValue(formState.map(x => x.field1));
  ...
}

And while this would be ok, it's not obvious to be done:

function MyComponent() {
  const value = useRecoilValue(formState.map(useCallback(x => x.field1, [])));
  ...
}

[Shmew]

Yeah that is a challenge.

One could argue it should be apparent as that's how React handles this issue already, but I do see the point. Especially since there are things like selectorFamily and atomFamily that behave similarly and don't have this drawback.

I think it really comes down to how you all want to handle it. In my opinion, good documentation noting this caveat would be sufficient while still enabling this feature. From what I've seen so far, one of the most common criticisms/complaints about recoil is the verbosity when creating additional simple selectors.

[drarmstr]

One option might be a lint rule, though we should hesitate to rely on that...

I mean, it would be fun to offer a second parameter function for a set function, mapAll variants, &c. But, we also need to carefully consider if it’s worth complicating the API surface area and creating multiple ways of doing the same thing... I can really see where this would appeal to people who prefer a more functional style, though.

[Shmew]

Yeah once you have map and bind you can build a whole variety of functions to make transformations easier:

• apply - RecoilValue<'T -> 'U> -> RecoilValue<'T> -> RecoilValue<'U>
• map2 ...mapX - (mapper: 'T -> 'U -> 'V) -> RecoilValue<'T> RecoilValue<'U> -> RecoilValue<'V>
• zip ...zipX - RecoilValue<'T> -> RecoilValue<'U> -> RecoilValue<'T * 'U>
• unzip ...zipX - RecoilValue<'T * 'U> -> RecoilValue<'T> * RecoilValue<'U>
• traverse - RecoilValue<'T -> 'U> -> RecoilValue<'T> [] -> RecoilValue<'U []>
• sequence - RecoilValue<'T> [] -> RecoilValue<'T []>

Since I have a library for bindings, I already implemented these types of features on-top of recoil itself. So this issue was mostly because I thought others not using my esoteric language would appreciate having a map function. Perhaps a companion library or additional import for things outside of simple map would be better?

[drarmstr]

Current status for this is that we have an implementation for a map() in #319. However, we don't want to merge it yet because it encourages risky usage of mapping during a render function. Without memoizing the map function this would lead to a new selector with every render. We tossed around some ideas, but currently thinking of just adding a simple lint rule to protect against this.

I toyed with memoizing the selector for the map based on a serialization of the mapping function instead of reference equality. This would allow .map()to be used from a render function. However, it doesn't capture closure state, so really isn't safe. I'm thinking of converting that approach to perhaps use it for some dynamic run-time warning for inappropriate use of .map()...

Anyway, if anyone is willing to help create the lint rule for use of .map() during a render function, (or really any creation of an atom or selector during render), that is all that is blocking merging this.

[BenjaBobs]

But, we also need to carefully consider if it’s worth complicating the API surface area and creating multiple ways of doing the same thing...

I think this is a really important thing to consider, and given the nature of react and the community as a whole, I don't think a map function as described here lends itself well to the principle of least surprise.

I like the idea of having a shorthand function to accomplish this, but I think adding shorthand syntax to an api should either not affect or, best case scenario, reduce the likelyhood of using the api wrong. I think in this case, it might slightly increase the chance of it being used wrong and causing bad performance.

On another note I think to remain in Recoil terminology, .select() might be a better name for the function, as it would better describe what is actually going on. Or maybe even .createSelector().

[Shmew]

I think this is a really important thing to consider, and given the nature of react and the community as a whole, I don't think a map function as described here lends itself well to the principle of least surprise.

I get where you're coming from there, but at the same time I think it's quite well stressed in the React documentation that calling non-hooks inside the body of your render function will not lead to a good time. So I wouldn't expect it to be much of a surprise to anyone.

The only real point of contention here is that this will still have a performance detriment when used inside useEffect. The question then is why are they even using recoil there? You can't share the recoil value, and they can just use useMemo for caching a transformed result.

On another note I think to remain in Recoil terminology, .select() might be a better name for the function, as it would better describe what is actually going on. Or maybe even .createSelector().

I don't really agree, map is a pretty well known term in js land as far as non-functional languages go, and is already part of the API is several other places. I've never really seen select for this purpose used outside of C#'s LINQ.

@drarmstr is there a resource or something you could point me to what would be needed for the linting rule? I might be willing to take a crack at it if it's simple enough.

[BenjaBobs]

I get where you're coming from there, but at the same time I think it's quite well stressed in the React documentation that calling non-hooks inside the body of your render function will not lead to a good time. So I wouldn't expect it to be much of a surprise to anyone.

The only real point of contention here is that this will still have a performance detriment when used inside useEffect. The question then is why are they even using recoil there? You can't share the recoil value, and they can just use useMemo for caching a transformed result.

I agree with you about the usage, I just worry that it will seem too intuitive for people to use it that way and not bother checking up on how it should actually be used. I think it should at least be addressed in the documentation then.

[drarmstr]

@Shmew - The AST Explorer is a great tool for testing out AST rules for ESLint. The rules also need tests with ESLint Tester using Jest. The tricky part is probably just determining if we are in the context of a render function or effect. Perhaps we could start more conservatively and cover any function..