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

Add batchedEffect() #69

Merged
merged 3 commits into from
May 21, 2024

Conversation

justinfagnani
Copy link
Contributor

@justinfagnani justinfagnani commented May 20, 2024

This adds a batchedEffect(cb) utility that synchronously runs its callback when dependencies are updated inside a batch() call.

const a = new Signal.State(0);
const b = new Signal.State(0);

batchedEffect(() => {
  console.log("a + b =", a.get() + b.get());
});

// Logs: a + b = 0

batch(() => {
  a.set(1);
  b.set(1);
});

// Logs: a + b = 2

* Runs a function inside a batch, and calls all the effected batched effects
* synchronously.
*/
export const batch = (fn: () => void) => {
Copy link
Collaborator

@NullVoxPopuli NullVoxPopuli May 20, 2024

Choose a reason for hiding this comment

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

why const arrow instead of function batch() {}?
(is this personal preference, or some perf/debugging thing I don't know about? 😅 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personal preference - I just use arrow functions for everything possible.

I supposed there's a slight theoretical perf benefit from not having a mutable binding, if the VM correctly elides the TDZ check for this case. Maybe some bundlers take advantage of this too? There's also of course a theoretical perf hit from un-elided TDZ checks.

I just personally think that arrow functions are the correct function semantics, and use the function to highlight cases where this might be non-lexical.

@justinfagnani justinfagnani marked this pull request as ready for review May 21, 2024 06:24
@NullVoxPopuli NullVoxPopuli merged commit 752e8f9 into proposal-signals:main May 21, 2024
4 checks passed
@NullVoxPopuli NullVoxPopuli added the enhancement New feature or request label May 21, 2024
* The effect also runs asynchronously, on the microtask queue, if any of the
* signals it depends on have been updated outside of a `batch()` call.
*/
export const batchedEffect = (effectFn: () => void) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, we should return an unsubscribe function here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

womp, want to throw up another PR? <3

@github-actions github-actions bot mentioned this pull request May 21, 2024
export const batch = (fn: () => void) => {
batchDepth++;
try {
// Run the function to notifiy watchers
Copy link

@lennartbuit lennartbuit May 24, 2024

Choose a reason for hiding this comment

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

typo: notify

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants