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

feat: synchronous atomEffect #53

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Conversation

dmaskasky
Copy link
Member

Related Discussion

#52

Summary

Refactor atomEffect to be synchronous. Synchronous atom effects run in the current task before store subscribers (such as store.sub and atom.onMount.

TODO

  • check tests
  • update docs

package.json Outdated
@@ -75,7 +76,7 @@
"html-webpack-plugin": "^5.5.3",
"jest": "^29.7.0",
"jest-environment-jsdom": "^29.7.0",
"jotai": "2.10.3",
"jotai": "https://pkg.csb.dev/pmndrs/jotai/commit/3b435d79/jotai",
Copy link
Member Author

Choose a reason for hiding this comment

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

FIXME: replace with official version once available.

@dmaskasky dmaskasky force-pushed the synchronous-atom-effect branch 4 times, most recently from 0d23697 to 05e030f Compare January 7, 2025 10:49
Copy link

codesandbox-ci bot commented Jan 7, 2025

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.

@dmaskasky dmaskasky force-pushed the synchronous-atom-effect branch 2 times, most recently from e8c8dd3 to 2101048 Compare January 7, 2025 10:59
@sebinsua
Copy link

sebinsua commented Jan 8, 2025

Is this published anywhere?

I think I ran into this 'problem' in the following situation:

  1. I had an observable source that could sometimes eventually emit some data.
  2. I had an effect that would subscribe to this and 'resolve' a Promise.withResolvers deferred with this data (or 'resolve' with empty data if a timeout passed), and which also handled cleanly unsubscribing, etc.
  3. An async atom used this effect and then awaited the deferred.promise so that our React code would attempt to block until it became available.
  4. The deferred promise never resolved as the effect was never called.

I could be wrong and my issue may have had nothing to do with this. But, either way, I'd be interested to find out if I could remove the slightly strange logic I wrote to make things work.

@dmaskasky
Copy link
Member Author

Could you make a simple reproduction of this logic in a codesandbox? I'm wrapping this PR up quickly and can test the new build against what you have there.

I think what you have sounds reasonable. Currently, atomEffects have a microtask delay from when their atom dependency changed (e.g the atom was set), to when the effect runs. This PR removes that delay.

But the effect's get and set are already both synchronous. I think you need the deferred promise because you are waiting for next signal, which is reasonable.

So for next steps, let me see what you currently have in a codesandbox.

@sebinsua
Copy link

sebinsua commented Jan 8, 2025

@dmaskasky Something like this?

https://codesandbox.io/p/sandbox/spring-glade-hvrczc?file=%2Fsrc%2FApp.js%3A31%2C1

Btw, is there any difference between wrapping an atom with withAtomEffect to run effects on mount/unmount of an atom and just using get(someEffect) within the 'read' of an atom? I wonder if I'm just misunderstanding how things are meant to be used sometimes.

@dmaskasky
Copy link
Member Author

dmaskasky commented Jan 8, 2025

@dmaskasky Something like this?

https://codesandbox.io/p/sandbox/spring-glade-hvrczc?file=%2Fsrc%2FApp.js%3A31%2C1

Btw, is there any difference between wrapping an atom with withAtomEffect to run effects on mount/unmount of an atom and just using get(someEffect) within the 'read' of an atom? I wonder if I'm just misunderstanding how things are meant to be used sometimes.

Ah, I see what's going on. Let's move this to a GH discussion. #54

@dmaskasky dmaskasky force-pushed the synchronous-atom-effect branch 8 times, most recently from 51b4913 to e774763 Compare January 17, 2025 18:20
@dmaskasky dmaskasky force-pushed the synchronous-atom-effect branch 2 times, most recently from 9fa56d1 to ab6d12a Compare January 23, 2025 07:09
@dmaskasky dmaskasky force-pushed the synchronous-atom-effect branch from 9209b91 to d9cebeb Compare January 23, 2025 15:39
@dmaskasky dmaskasky force-pushed the main branch 9 times, most recently from af2e616 to a39282a Compare January 25, 2025 23:36
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.

3 participants