FBCM-4170 Don't raise from a query #172
Conversation
It seems like a query causing a raise can initiate an infinite loop. When we call `setSelection` everywhere, we always get the same behavior. That's good for having uniform behavior. Unfortunately the way this was setup seemed to cause an infinite loop sometimes. If a parent told either `Ocelot.Components.DatePicker.Component.SetSelection _` or `Ocelot.Components.TimePicker.Component.SetSelection _`, they would end up raising `Ocelot.Components.DatePicker.Component.SelectionChange _`, or `Ocelot.Components.TimePicker.Component.SelectionChange _`, respectively. Depending on how the parent handled that, it could end up telling the same query and kicking off an infinite loop. We try to address that by not raising the query we just responded to only in the case where we just received the query. The other cases still should still raise their output as before since they aren't initiated from the query. Cc: Gabe Johnson <gabe@citizennet.com>
joneshf-cn
requested review from
a team and
davezuch
and removed request for
a team
davezuch
left a comment
davezuch
left a comment
There was a problem hiding this comment.
Thanks for doing this! Just one non-blocking suggestion below.
| setSelectionWithoutRaising :: forall m. MonadAff m => Maybe Date -> CompositeComponentM m Unit | ||
| setSelectionWithoutRaising selection = do | ||
| st <- H.get | ||
| let targetDate = maybe st.targetDate (\d -> (year d) /\ (month d)) selection | ||
| H.modify_ _ { selection = selection, targetDate = targetDate } | ||
| synchronize | ||
|
|
There was a problem hiding this comment.
I'm weary of the potential for the setSelection logic having to change and the engineer not realizing they should also change setSelectionWithoutRaising.
What do you think of updating setSelection to something like:
setSelection :: forall m. MonadAff m => Maybe Date -> CompositeComponentM m Unit
setSelection selection = do
setSelectionWithoutRaising selection
H.raise (SelectionChanged selection)I don't think the order of when synchronize is called is important here. I believe that just updates the calendar view to match the selected date, so it seems fine to call it before raising.
There was a problem hiding this comment.
Makes sense to me. It seems like synchronize just messes around with the state, so I agree that it probably doesn't matter if it's called before or after the raise. Honestly, it feels like it should all happen before the raise anyway, and likely it's a hold-over from the halogen 5 conversion. But, I didn't look into it that closely.
| setSelectionWithoutRaising :: forall m. Maybe Time -> CompositeComponentM m Unit | ||
| setSelectionWithoutRaising selection = do | ||
| H.modify_ _ { selection = selection } | ||
| synchronize |
We don't want to have to maintain two different code paths for doing mostly the same thing. We want to avoid a developer changing one in the future without changing the other. We update `setSelection` to defer to the logic in `setSelectionWithoutRaising` before raising.
What does this pull request do?
We stop raising the query we just responded to when we receive it in the
Ocelot.Components.DatePicker.ComponentandOcelot.Components.TimePicker.Componentcomponents.Where should the reviewer start?
purescript-halogen-selectthing works.ordersandwildcatrepos, requesting a review from both teams.