fix subs bug #15680
Merged
fix subs bug #15680
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jenkins BuildsClick to see older builds (4)
|
flexsurfer
commented
Apr 18, 2023
| @@ -43,7 +43,7 @@ | |||
| [:f> | |||
| (fn [] | |||
| (let [duration (or (rf/sub [:toasts/toast-cursor id :duration]) 3000) | |||
| on-dismissed #((or (rf/sub [:toasts/toast-cursor id :on-dismissed]) identity) id) | |||
| on-dismissed (or (rf/sub [:toasts/toast-cursor id :on-dismissed]) identity) | |||
ilmotta
approved these changes
Apr 18, 2023
| @@ -80,7 +80,7 @@ | |||
| (create-timer)))))] | |||
| ;; create auto dismiss timer, clear timer when unmount or duration changed | |||
| (rn/use-effect (fn [] (create-timer) clear-timer) [duration]) | |||
| (rn/use-unmount on-dismissed) | |||
| (rn/use-unmount #(on-dismissed id)) | |||
There was a problem hiding this comment.
Why do we need to pass the id here and create the anonymous function? It's questionable because the subscription [:toasts/toast-cursor id :on-dismissed] already returns a function that has access to the id value and the identity function doesn't care about any argument in this case.
I'm just wondering why it wouldn't work using just (rn/use-unmount on-dismissed).
There was a problem hiding this comment.
It's just to handle situations where id and on-dismissed are both dynamically generated and hard or not elegant enough to pass the id to the on-dismissed fn.
It might be a premature optimization.
J-Son89
approved these changes
Apr 18, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #15678
sometimes you can see in re-frisk subscriptions run in the loop, that means there is a mistake in usage, and we should find and fix that
PS: actually it didn't solve the problem, so the review is on hold
PS2: fixed , so subscription must be called only by reagent when its initializing the view, but in this case it was run in the dismiss function outside of reagent context , which leads to subscription leak
ready for review