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

Allow for nested effect() calls #4

Merged
merged 3 commits into from
Nov 13, 2023
Merged

Allow for nested effect() calls #4

merged 3 commits into from
Nov 13, 2023

Conversation

amatiasq
Copy link
Collaborator

Added tests to signals

  • should work with async effects
  • should work without race conditions between async effects and signals

The second one failed and was solved by switching the effect / signal communication from a single variable to a stack. This allows for nested effect() invocations.

The race condition is, in practice, the same as nested effect() calls: if the first call has asynchronous code it will finish after the second effect() has been invoked. When effect stored the current function in a single variable this meant the second effect() will override current before the first effect() was completed.

Please merge this with "Squash" operation as the commit messages aren't elaborated.

@amatiasq
Copy link
Collaborator Author

In the last commit, which can be dropped and the implementation will still work, I changed the signature of effect() from (fn: Effect) => void to (fn: Effect) => Promise. This means we can wait for asynchronous effects:

await effect(() => {
  return new Promise(resolve => setTimeout(resolve, 10 * 1000)) 
});

// execution continues after 10 seconds;

Not sure when or how this may be useful so feel free to drop it. On the other hand it makes no harm, only some overhead.

@amatiasq amatiasq requested a review from aralroca November 13, 2023 18:39
function removeFromStack(fn: Effect) {
const index = stack.lastIndexOf(fn);
if (index !== -1) stack.splice(index, 1);
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I regret using lastIndexOf() here. I planned to store the stack in the other direction when I wrote this.
Probably indexOf() would be more accurate.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, it make sense, I changed here 00c4e2c thanks!

Copy link
Collaborator

@aralroca aralroca left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @amatiasq. At the moment this is fine as a solution. It's a very rare case and I'm still wondering if we really want to support this bad practice by adding more bytes to the client code, instead of improving the development experience to put warnings of bad practices and documentation on how to do it correctly.

But since it doesn't add much extra kb, maybe it's ok, at least now it works better, although I don't see it as the most optimal solution if we want to support this. Maybe the effects would be isolated? That is, it makes sense that they run several times? If the parent effect when running also makes the child effect run then it always makes an extra call to the child?

Copy link
Collaborator

@aralroca aralroca left a comment

Choose a reason for hiding this comment

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

For now I merge it up, but it would be good to think about the alternative I mention.

@aralroca aralroca merged commit 8d63c2c into main Nov 13, 2023
1 check passed
@aralroca aralroca deleted the signal-race-condition branch November 13, 2023 21:31
@aralroca
Copy link
Collaborator

@all-contributors please add @amatiasq for code

Copy link
Contributor

@aralroca

I've put up a pull request to add @amatiasq! 🎉

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.

2 participants