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

onBecome(Un)Observed intercept hooks can remove each other #1537

Closed
jamiewinder opened this issue May 8, 2018 · 5 comments · Fixed by #1833
Closed

onBecome(Un)Observed intercept hooks can remove each other #1537

jamiewinder opened this issue May 8, 2018 · 5 comments · Fixed by #1833
Labels

Comments

@jamiewinder
Copy link
Member

The way onBecomeObserved and onBecomeUnobserved are implemented means they can inadvertently cancel each other.

Source of the issue is here:
https://github.com/mobxjs/mobx/blob/master/src/api/become-observed.ts#L44

If you add a hook, the behaviour is 'call the original hook, then the new one'. The disposer function that is returned has the behavior of 'restore the original hook'.

This means that if a hook is disposed (with the return value), any subsequent hooks added since that was added will also be removed. Example here:
https://codesandbox.io/s/k0y70x7vy7

(You'd expect one of the two callbacks be called, but it isn't because the first was removed).

I started a fix for this, but the solution wasn't as immediately straightforward as I thought and since I don't know when I'll have time to give it another go I wanted to record this for now.

@michalwarda
Copy link

Hey @mweststrate I'll be happy to try to fix this issue. Is there anything I should take into account before doing that? Any suggestions or we can discuss it after I try to implement it?

@mweststrate
Copy link
Member

mweststrate commented Oct 10, 2018 via email

@michalwarda
Copy link

@mweststrate Do you have any suggestions on how can we continue this? I have some time currently so I can adjust the PR.

fi3ework added a commit to fi3ework/mobx that referenced this issue Dec 6, 2018
fi3ework added a commit to fi3ework/mobx that referenced this issue Dec 6, 2018
fi3ework added a commit to fi3ework/mobx that referenced this issue Dec 7, 2018
mweststrate added a commit that referenced this issue Dec 12, 2018
fix: earlier onBecome(Un)Observed disposer will not dispose later listener (fix #1537)
@mweststrate
Copy link
Member

released in 4.8.0 / 5.8.0

@lock
Copy link

lock bot commented Jul 21, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs or questions.

@lock lock bot locked as resolved and limited conversation to collaborators Jul 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
3 participants