Publicly allow dispose() on an effect callback #697
Conversation
🦋 Changeset detectedLatest commit: 5c388a1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for preact-signals-demo ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Size Change: +37 B (+0.04%) Total Size: 87.6 kB
ℹ️ View Unchanged
|
JoviDeCroock
force-pushed
the
allow-dispose-publicly
branch
from
3d69e8b to
62ca0fa
Compare
| a.value = "aaa"; | ||
| expect(spy).to.be.calledOnce; | ||
| }); | ||
|
|
There was a problem hiding this comment.
Nit: Let's add a test where dispose() is called from inside the callback and after that the returned dispose function is called. Just to ensure that we're not erroring when it's called twice
| Effect.prototype.dispose = function () { | ||
| this._dispose(); | ||
| }; |
There was a problem hiding this comment.
This would be great 🙏 I've got a couple of places in @dnd-kit where I need to set up temporary effects and tear them down from within the effect after a condition is met, for example:
effect(() => {
if (manager.dragOperation.status.value === 'dropped') {
this.dispose();
performDropAnimation();
}
});Right now the only way to safely do this is:
const dispose = effect(() => {
if (manager.dragOperation.status.value === 'dropped') {
queueMicrotask(() => dispose());
performDropAnimation();
}
});| }); | ||
| const dispose = effect(function () { | ||
| spy(); | ||
| this.dispose(); |
There was a problem hiding this comment.
Super tiny nit: Maybe also a test where you call spy() just after dispose()
This enables us to access
this.dispose()within the callback of an effect. We needed to do some typing shenanigans to allow for an unspecifiedthislike we need in our adapter packages so we can override_startand others.Resolves #395