Reflex.Collection.listWithKey samples Dynamic values too late

[bidigo]

We have an issue in our app where we get annoying flicker in the UI whenever we use list or simpleList from Reflex.Collection. We're using reflex 0.8.0.0 and building with the latest obelisk.

We traced the culprit to the usage of Map.empty in listWithKey:

reflex/src/Reflex/Collection.hs

Lines 75 to 110 in 9b721a5

	listWithKey
	:: forall t k v m a
	. (Ord k, Adjustable t m, PostBuild t m, MonadFix m, MonadHold t m)
	=> Dynamic t (Map k v)
	-> (k -> Dynamic t v -> m a)
	-> m (Dynamic t (Map k a))
	listWithKey vals mkChild = do
	postBuild <- getPostBuild
	let childValChangedSelector = fanMap $ updated vals
	-- We keep track of changes to children values in the mkChild
	-- function we pass to listHoldWithKey The other changes we need
	-- to keep track of are child insertions and
	-- deletions. diffOnlyKeyChanges keeps track of insertions and
	-- deletions but ignores value changes, since they're already
	-- accounted for.
	diffOnlyKeyChanges olds news =
	flip Map.mapMaybe (align olds news) $ \case
	This _ -> Just Nothing
	These _ _ -> Nothing
	That new -> Just $ Just new
	rec sentVals :: Dynamic t (Map k v) <- foldDyn applyMap Map.empty changeVals
	let changeVals :: Event t (Map k (Maybe v))
	changeVals =
	attachWith diffOnlyKeyChanges (current sentVals) $ leftmost
	[ updated vals
	-- TODO: This should probably be added to the
	-- attachWith, not to the updated; if we were using
	-- diffMap instead of diffMapNoEq, I think it might not
	-- work
	, tag (current vals) postBuild
	]
	listHoldWithKey Map.empty changeVals $ \k v ->
	mkChild k =<< holdDyn v (select childValChangedSelector $ Const2 k)

Basically, what seems to happen is that the value of the Dynamic vals map argument is only used at the PostBuild time instead of being sampled and used in the call to the listHoldWithKey. In our example this leads to a list widget in our UI to initially get rendered as an empty list and then at the next frame to get filled with data, which scrolls all the other widgets that come after it.

We can confirm the problem goes away when using the sampled value of vals when calling listHoldWithKey.

Is this usage of Map.empty intentional, perhaps because of performance reasons? If not I could go ahead and prepare a patch for this.