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

Recurse #29

Merged
merged 1 commit into from
Jan 22, 2024
Merged

Recurse #29

merged 1 commit into from
Jan 22, 2024

Conversation

dmaskasky
Copy link
Member

@dmaskasky dmaskasky commented Jan 22, 2024

Related Issue: #26
Related Discussion: #16

This PR introduces additional api that allows the developer to opt-in for recursion.

type SetterWithRecurse = Setter && { recurse: Setter }
type EffectFn = (get: Getter, set: SetterWithRecurse) => CleanupFn

✅ Synchronous recursive loops

const effect = atomEffect((get, set) => {
  const count = get(countAtom)
  if (count < 5) {
    set.recurse(countAtom, increment)
  }
})
store.sub(effect, () => void 0)
await delay(0)
store.get(countAtom) // 5

✅ Asynchronous recursive loops. Both task and microtask delay is supported.

atomEffect((get, { recurse }) => {
  const count = get(countAtom)
  if (count < 5) {
    Promise.resolve().then(() => {
      recurse(countAtom, increment)
    })
  }
})
store.sub(effect, () => void 0)
await delay(0)
store.get(countAtom) // 5

❌ Synchronous recursive loops in callback are disallowed (with warning)

atomEffect((get, set) => {
  get(countAtom)
  return () => {
    set.recurse(countAtom, increment)
  }
})

Copy link

codesandbox-ci bot commented Jan 22, 2024

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 5d99d3d:

Sandbox Source
React Configuration
React TypeScript Configuration

@dmaskasky dmaskasky force-pushed the recurse branch 7 times, most recently from 56718fa to d1805d1 Compare January 22, 2024 08:02
Copy link
Member

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, I'm not a big fan of complicating atomEffect.
If I were to do it, I'd create a separate atomRecurseEffect.
Having that said, it's up to you.


const initAtom = atom(null, (get, set, mounted: boolean) => {
const ref = get(refAtom)
ref.mounted = mounted
if (mounted) {
ref.set = set
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a hack that I would avoid as much as possible. It's a valid hack for now though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was simply no other way. After the first recursion, the effectFn and setter must be run synchronous so that it can recurse down the call stack. So we have to use set instead of setSelf.

@dmaskasky
Copy link
Member Author

@dai-shi

In general, I'm not a big fan of complicating atomEffect. If I were to do it, I'd create a separate atomRecurseEffect. Having that said, it's up to you.

Yeah, its a bit complicated now. This is why I added so many additional tests to back this up. atomRecurseEffect sounds interesting, but I wanted the ability to mix set and recurse. I also like the fact that the default behavior is to prevent infinite loop footguns.

Copy link
Member

@himself65 himself65 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update readme

warnSpy.mockRestore()
})

// FIXME: is there a way to disallow asynchronous infinite loops in cleanup?
Copy link
Member Author

@dmaskasky dmaskasky Jan 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dai-shi I couldn't find a way to detect and prohibit async recursion in the cleanup function.

atomEffect((get, set) => {
  get(watchedAtom)
  return () => {
    setTimeout(() => {
      set.recurse(watchedAtom, increment)
    }, 1000)
  }
})

IMO, recursion in the cleanup function is counter-intuitive and should not be allowed. The problem I ran into is how, technically, to distinguish it from async recursion in the effect function.

atomEffect((get, set) => {
  get(watchedAtom)
  setTimeout(() => {
    set.recurse(watchedAtom, increment)
  }, 1000)
})

I was wondering if you had any ideas.

@dmaskasky dmaskasky merged commit 23f39d0 into main Jan 22, 2024
3 checks passed
@dmaskasky dmaskasky deleted the recurse branch January 22, 2024 19:15
dmaskasky added a commit that referenced this pull request Jan 22, 2024
@QWu4xYV
Copy link

QWu4xYV commented Jan 24, 2024

Thanks for implementing this so quickly! What's the typical cadence for releases? Excited to try it out :)

@dmaskasky
Copy link
Member Author

I'll release it today. Sorry for the delay. I was waiting on an unrelated bug fix in jotai core to release, but its taking longer than expected.

@QWu4xYV
Copy link

QWu4xYV commented Jan 25, 2024

Oh, no worries, thanks for the update! If you do want to wait, that's okay too

@dmaskasky
Copy link
Member Author

dmaskasky commented Jan 25, 2024 via email

@QWu4xYV
Copy link

QWu4xYV commented Jan 28, 2024

Works perfectly for my use case, thanks so much!

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.

4 participants