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

atom family util #45

Merged
merged 15 commits into from
Sep 20, 2020
Merged

atom family util #45

merged 15 commits into from
Sep 20, 2020

Conversation

dai-shi
Copy link
Member

@dai-shi dai-shi commented Sep 15, 2020

Close #23

  • example snippet
  • example codesandbox
  • docs/utils.md (how to use areEqual for recoil-like-behavior)
import { atomFamily } from 'jotai/utils'

const todoFamily = atomFamily((name) => name)

  todoFamily('foo')
  // this will create a new atom('foo'), or return the one if already created
import { atomFamily } from 'jotai/utils'

const todoFamily = atomFamily(
  (name) => (get) => get(todosAtom)[name],
  (name) => (get, set, arg) => {
    const prev = get(todosAtom)
    return { ...prev, [name]: { ...prev[name], ...arg } }
  }
)
import { atomFamily } from 'jotai/utils'

const todoFamily = atomFamily(
  ({ id, name }) => ({ name }),
  null,
  (a, b) => a.id === b.id
)

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 15, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 98052e8:

Sandbox Source
React Configuration
React Typescript Configuration

@dai-shi
Copy link
Member Author

dai-shi commented Sep 15, 2020

I'm not really happy with replicating types from atom(). Are there any better ways, including fixing types of atom()?

@brookslybrand
Copy link

So sorry @dai-shi, this took me longer than I expected to test. I kept having issues linking the package to my demo, and basically had to end up hacking it together.

That being said, it was actually really easy to migrate! I ended up removing a net of 49 lines with Jotai from Recoil. I want to get the diff up on GitHub to be able to share at some point.

The only thing I'm really not sure about is the remove method. I think it could be a bit confusing, because if you call myAtomFamily.remove(params), it will remove the reference inside of the map, but it's not deleting the atom itself. So if you just call that but you still have a hook like useAtom(myAtomFamily(params) setup, you'll immediately add the atom back. Maybe this is an expected result, but I found it a bit confusing and not really useful since I had to coordinate it's removal along with the actual unmounting of the component using the atom.

I can try and whip up a simple example to show you what I mean if that was at all confusing.

Otherwise great work! I'm really excited for when this gets officially pushed in, definitely makes me want to try jotai more!

@dai-shi
Copy link
Member Author

dai-shi commented Sep 17, 2020

Sounds great! For remove(), I'm not sure either. I just feel bad without it, because a user can't avoid memory leak. We could try something like gc(), if that makes more sense.

@dai-shi
Copy link
Member Author

dai-shi commented Sep 18, 2020

remove()

Now, I feel like remove(param) is better than remove(atom).

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 19, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 96a7fa8:

Sandbox Source
React Configuration
React Typescript Configuration

@dai-shi
Copy link
Member Author

dai-shi commented Sep 19, 2020

Hmm,

type T = typeof atom extends (read: number) => infer R ? R : never  

This can't infer type properly.

@dai-shi
Copy link
Member Author

dai-shi commented Sep 19, 2020

Finally made a working and fully typed demo.
https://codesandbox.io/s/react-typescript-forked-8zfrn?file=/src/App.tsx

@brookslybrand
Copy link

brookslybrand commented Sep 19, 2020

Glad you got a demo up, it's looking good!

I definitely get the desire to have a remove and the worry about a memory leak that the a user will face. I still think people aren't necessarily going to realize or get used to the fact that they'll need to both remove the atom from the atomFamily, as well as from some other atom that likely holds it's params (or however they're creating and storing the params).

Now, I feel like remove(param) is better than remove(atom).

I totally agree, and I didn't even realize it was the other way 😅 I was trying to do it with the param initially, so it wasn't even working. It's a bit difficult though to know whether or not you've removed the atom, because by checking you'll basically just recreate the atom.

Overall though, great work! Let me know if there's anything I can do to help push this in

@dai-shi dai-shi merged commit de49353 into master Sep 20, 2020
@dai-shi dai-shi deleted the atom-family branch September 20, 2020 11:09
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.

Is atomFamily behavior possible?
2 participants