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

Is atomFamily behavior possible? #23

Closed
brandonculver opened this issue Sep 10, 2020 · 22 comments · Fixed by #45
Closed

Is atomFamily behavior possible? #23

brandonculver opened this issue Sep 10, 2020 · 22 comments · Fixed by #45
Labels
has snippet This issue includes code snipppets as solutions or workarounds help wanted Please someone help on this

Comments

@brandonculver
Copy link

Recoil has atomFamily which allows atoms to be created and referenced with something akin to a groupId. It seems like that kind of behavior can not be replicated without some kind string key identifiers.

@dai-shi
Copy link
Member

dai-shi commented Sep 10, 2020

Hi, nice to see this issue is opened for discussion.

https://recoiljs.org/docs/api-reference/utils/atomFamily
Reading the doc, I'm not sure if I understand groupId can reference atoms.
Oh, atoms created with atomFamily can be referenced by a name.

The base form would be something like:

const atomFamily = (initialValue) => {
  const map = {}
  return (name) => {
    if (!map[name]) {
      map[name] = atom(initialValue)
    }
    return map[name]
  }
}

@brandonculver
Copy link
Author

Yeah, wasn't for sure if it could be accomplished so easily but I guess there is nothing preventing it.

@nksaraf
Copy link

nksaraf commented Sep 10, 2020

Have worked with this api and there could be an easy way to do it.. whatever key the user plans to use can be stringified and used to cache(as a simple object) atoms. Generate new ones when an atom with a new key is requested, otherwise return the reference to a preexisting atom from the cache

@dai-shi
Copy link
Member

dai-shi commented Sep 11, 2020

Has anyone tried if it (the code snippet in #23 (comment)) really works?

@dai-shi dai-shi added has snippet This issue includes code snipppets as solutions or workarounds help wanted Please someone help on this labels Sep 11, 2020
@kentcdodds
Copy link

If someone wants a case study to try this with, you could see if you could port this example to jotai: https://github.com/kentcdodds/react-performance/blob/e92b1a1b61dae6f852524dfe9f8b81511a55113c/src/final/06.extra-4.js#L15-L18

@gsimone
Copy link
Member

gsimone commented Sep 12, 2020

@dai-shi we could test this on the strand repo, I have nodes and fields that could be implemented with this 👍

@dai-shi
Copy link
Member

dai-shi commented Sep 12, 2020

https://github.com/kentcdodds/react-performance/blob/e92b1a1b61dae6f852524dfe9f8b81511a55113c/src/final/06.extra-4.js#L15-L18

const cellAtoms = atomFamily({
  key: 'cells',
  default: ({row, column}) => initialGrid[row][column],
})

So, this requires supporting function as an initializer.
And, we use Map instead.

const atomFamily = (initializer) => {
  const map = new Map()
  return (params) => {
    if (!map.has(params)) {
      map.set(params, atom(typeof initializer === 'function' ? initializer(params) : initializer)
    }
    return map.get(params)
  }
}

(Supporting async initializer would require more work..

@dai-shi
Copy link
Member

dai-shi commented Sep 13, 2020

https://github.com/kentcdodds/react-performance/blob/e92b1a1b61dae6f852524dfe9f8b81511a55113c/src/final/06.extra-4.js#L82

Ah, it's calling the function with referentially different objects.
We'd need to shallow compare (or deep compare) or stringify the params. (Is JSON.stringify safe for this purpose?)

@dai-shi
Copy link
Member

dai-shi commented Sep 13, 2020

Maybe, use https://lodash.com/docs/4.17.15#isEqual ?

import isEqual from 'lodash.isEqual'

const atomFamily = (initializer) => {
  const atoms = []
  return (params) => {
    const found = atoms.find(x => isEqual(x[0] === params))
    if (found) {
      return found[1]
    }
    const newAtom = atom(typeof initializer === 'function' ? initializer(params) : initializer)
    atoms.push([params, newAtom])
    return newAtom
  }
}

btw, is there a way to remove an atom from the family?

@brookslybrand
Copy link

brookslybrand commented Sep 14, 2020

Using JSON.stringify will unfortunately not work, as changing the order of the keys will technically result in a different param, whereas Recoil would consider this the same param. You can see the bug in this code sandbox. Clicking "Change me!" in the recoil button will update both atoms (really the same atom), whereas in the Jotai example it will consider them as 2 different atoms.

You can see that I commented out your solution using isEqual from lodash.isequal, and it works properly.

Only issue I see with this example is the size of lodash.isequal. I could be missing something, but adding 3.8kB doesn't seem great for this one feature.

An alternative library I've used in the past with no problems is fast-deep-equal, which is much smaller. There may be better solutions out there still.

Edit:
I hit "Comment" a little too early. I also wanted to point out the specific language in Recoil, just to make sure we match what they have (emphasis added by me):

The atomFamily essentially provides a map from the parameter to a atom. You only need to provide a single key for the atomFamily and it will generate a unique key for each underlying atom. These atom keys can be used for persistence, and so must be stable across application executions. The parameters may also be generated at different callsites and we want equivalent parameters to use the same underlying atom. Therefore, value-equality is used instead of reference-equality for atomFamily parameters. This imposes restrictions on the types which can be used for the parameter. atomFamily accepts primitive types, or arrays or objects which can contain arrays, objects, or primitive types.

@kentcdodds
Copy link

I'm a big fan of dequal which is very small (it's an intentional feature). I use it in use-deep-compare-effect and it's great 👍

@dai-shi
Copy link
Member

dai-shi commented Sep 14, 2020

Thanks SO much for doing it!

adding 3.8kB doesn't seem great for this one feature.

Didn't know it's that big.

Therefore, value-equality is used instead of reference-equality for atomFamily parameters.

OK, this is contradictory to Jotai's philosophy (= no string keys)

That said, for atomFamily, people would expect Recoil's behavior, and it's easy to use (compared to reference-equality + memoization), so using deep-equality would make sense.


(edit 1)

Is it too hard to write a simple deep-equal by our own?

Found another solution: https://github.com/substack/json-stable-stringify (not very small)


(edit 2)

const objDeepEqual = (o1, o2) => {
  if (o1 === o2) return true;
  if (typeof o1 !== "object" || typeof o2 !== "object") return false;
  const k1 = Object.keys(o1);
  const k2 = Object.keys(o2);
  if (k1.length !== k2.length) return false;
  return k1.every((k) => objDeepEqual(o1[k], o2[k]));
};

Should work for simple (JSON serializable) objects. Not sure how performant it is.

@brookslybrand
Copy link

Yeah, I don't see a problem with writing your own comparison function, it's just another thing that needs to be tested and compared

I dug into recoil's implementation a bit more last night after writing my comment. It looks like they use their own custom stringify function which they use for the keys of their atom cache.

I don't know if you would necessarily need to go this far, but it did make me realize that you probably don't want the user to pass in anything besides a primitives, object, or array as a parameter. For example (and maybe this really is the only exception worth noting) you probably want to ignore or throw an error on functions.

The function you supplied returns false for all functions it looks like, which is good, but there seem to be some problems with some other objects:

objDeepEqual(new Date(), new Date('2020-01-01')) // true
objDeepEqual(new Map(), new Map([['key', 'value']])) // true
objDeepEqual(new Set(), new Set([1, 2, 3])) // true

@dai-shi
Copy link
Member

dai-shi commented Sep 14, 2020

Wow, your research is so helpful.
Yeah, I don't want to support arbitrary values for this one.
Primitives, objects, arrays, and combination of them would work for many cases.
I was actually thinking if shallow equal covers 80% use cases. But, maybe not, because object array and array in object property would be often used.
Sure, we can guard from unexpected values. TS would also help.

The next challenge is how to deal with derived atoms, a.k.a. selectorFamily in Recoil.
It'd not be easy nor intuitive to let atomFamily handle everything.
Or, maybe we could, if we only accept "function" initializer.

const atomFamily = (initializeRead, initializeWrite) => {
  const atoms = []
  return (params) => {
    const found = atoms.find(x => deepEqual(x[0] === params))
    if (found) {
      return found[1]
    }
    const newAtom = atom(initializeRead(params), initializeWrite && initializeWrite(params))
    atoms.push([params, newAtom])
    return newAtom
  }
}

I'm also thinking about adding custom equal function at the third argument. (but, this means the default is ref equal, which may not cover many use cases.)

@brookslybrand
Copy link

Happy to help!

Yep, I think you're right, anything beyond those types isn't particularly useful or easy to reason about.

Yeah, I think adding the initializeWrite makes sense, just because it matches your atom API. I personally don't find selectorFamily that helpful, it's more just there to mirror atomFamily I think. Given that jotai doesn't have a select I would argue that you don't really need a selectorFamily.

I have a little demo app I used to play with/demonstrate Recoil, which uses atomFamily and selectorFamily: https://github.com/brookslybrand/react-optimizations/blob/main/src/app-state.js
I'm happy to try to rewrite it with the API you've proposed here and see what issues I run into.

As for adding the custom equal as a third argument, that could work, kind of like React.memo's areEqual argument. It does push it on the user to implement/provide their own library, but that might be okay,. Just need to make sure it's well documented in a recipe so people can truly get "recoil-like-behavior".

@dai-shi
Copy link
Member

dai-shi commented Sep 14, 2020

kind of like React.memo's areEqual argument. It does push it on the user to implement/provide their own library, but that might be okay

I'd go with it. For some case with string names, ref equality is fine. Even with the example by Kent, shallow equal works. We document usage for recoil-like-behavior.

I'm happy to try to rewrite it with the API you've proposed here and see what issues I run into.

That's great! I'll draft a PR then.

One remaining concern is if we need to provide a way to remove atoms from the family. Our current implementation only allow adding atoms.

@brookslybrand
Copy link

Awesome! Let me know when the PR is up and I'll try it out.

From everything I've seen and tried, I don't think there's a way to remove atoms from an atomFamily, so I really wouldn't worry about it. There certainly is the potential that someone could just continue adding new atoms forever and overload their memory, but that's going to be a pretty rare case and I imagine they'd be doing something a little bit unique that might have some better, or at least alternate solutions. So all that to say, I think it's fine without an atom removal feature.

@dai-shi
Copy link
Member

dai-shi commented Sep 15, 2020

@gsimone If you were to use this atomFamily in strand, would you need removability?

@dai-shi dai-shi mentioned this issue Sep 15, 2020
3 tasks
@dai-shi
Copy link
Member

dai-shi commented Sep 15, 2020

@brookslybrand #45 is ready for trial.

I implemented remove(fooAtom) too.

@italodeandra
Copy link
Contributor

italodeandra commented Sep 15, 2020

Hey guys, I don't want to interrupt the discussion but as I see both of you discussing about how the "compare" function of Recoil works, I can give you guys some insight.

@brookslybrand Is right about Recoil using their own stringify to make comparisons. That's because they found a more performant way of comparing objects by transforming them into array.

It works roughly like this: They get your object like this:

let person = {
  name: "Jackie Chan",
  age: 66
}

And transforming into:

[ "age", 66, "name", "Jackie Chan" ]

Then when you want compare them, you can go each by each items of the array (or you could even go for the JSON.stringify if the array is not that big). Their solution fixes the problem of using the JSON.stringify straight on the object, because as you can see on the array above, they sorted the properties (keys) of the object.

I hope this was enough clear to give you guys an insight. If not, don't be shy to ask.

@italodeandra
Copy link
Contributor

italodeandra commented Sep 15, 2020

By the way, I agree with you guys making just like React.memo to receive a comparison function instead of implementing the comparison function on the library. As I said on my comment above, you have different comparison solutions for each case, so I would rather have control over that.

@dai-shi
Copy link
Member

dai-shi commented Sep 15, 2020

@italodeandra Hey, thanks for chiming in.
It's interesting to see transforming an object into an array for performance.
In #45, I took the areEqual approach, so you could do as you want. (even the transformation would be possible with WeakMap.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has snippet This issue includes code snipppets as solutions or workarounds help wanted Please someone help on this
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants