Inconsistency with RequesterT after EventWriterT change

[oliver-batchelor]

EventWriterT recently changed:

https://github.com/reflex-frp/reflex/commit/ca50bb4cf1e85e1c980b3ddad0e1b0c483542349#diff-ca34969f303705bc74cc027f0eedb19f

Which after some digging I think is a more correct behaviour (even though it broke my code!). Except that RequesterT still has the old behaviour too, I think.

Code below, before change outputs:

("tellEvent: ","hi!") ("performEvent_: ","hi!")

Afterwards just:

("performEvent_: ","hi!")

  e <- getPostBuild
  (_, w) <- runEventWriterT $ void $ do
    flip runWithReplace (blank <$ e) $ do

      -- This has the effect of delaying e' from e
      (_, e') <- runWithReplace blank (return "hi!" <$ e)

      tellEvent e'
      performEvent_ (liftIO . print . ("performEvent_: ",) <$> e')

  performEvent_ (liftIO . print . ("tellEvent: ",) <$> w)

[JBetz]

Since upgrading to reflex-0.5, a lot of my widgets that use tellEvent are broken. The event only gets sent up the first time that it's fired.

For example:

dyn_ $ ffor stateD $ \(State _) -> do
  -- dom stuff
  tellEvent $ pure <$> select ZoomIn menuE
  tellEvent $ pure <$> select ZoomOut menuE

I can only "tell" either ZoomIn or ZoomOut once. Subsequent firings of matching menuE events have no effect.

Is this related?

[ryantrinkle]

@JBetz I believe @Saulzar's case would apply if stateD is changing at the same time that menuE is firing. However, it doesn't explain why you'd be seeing only a single tellEvent taking effect. Would you mind posting a bit more context? I'd like to see if I can reproduce the issue and get it fixed ASAP.

@Saulzar Yes, that semantic change was intentional, as I believe the previous behavior didn't make much sense. However, I may have made the wrong call to release it without some kind of deprecation cycle. Sorry for making you debug through this! I will think about what to do about RequesterT - thanks for bringing it up.

[JBetz]

@ryantrinkle I think it's an issue with tellEvent being used inside of a dynamic produced by listWithKey. Here's a MVE:

testW :: MonadWidget t m => m ()
testW = mdo
  let update [()] cur = cur + 1
      update _ cur    = cur
  counterD <- foldDyn update 0 counterE
  (_, counterE) <- runEventWriterT $
    listWithKey (Map.singleton 0 <$> counterD) $ \_ counterD -> do
      dynText $ pack . show <$> counterD
      incE <- button "+"
      tellEvent $ pure <$> incE
  pure ()

The value only increments after the first time you press the + button.

[oliver-batchelor]

Actually if stateD and menuE fire at the same time I believe the tellEvent is still getting propagated (Given the description of the change on the commit I thought perhaps it shouldn't?).

In my test above the tellEvent is only getting cut off if the event is delayed from the switch (caused by the runWithReplace).

[JBetz]

The EventWriterT change is the cause of #234 as well.

[oliver-batchelor]

Much like the different variations in switch/switchPromptly/switchPromptlyOnly (and much as it is painful to have umpteen variations of the same thing), is it reasonable to have both prompt/non prompt switching EventWriterT?

Personally I prefer non prompt switching to be a more intuitive default, it also has an advantage of being simpler (and more performant).

[ryantrinkle]

Yes, but for what it's worth, switchPromptlyOnly should be the only version that exists; switchPromptly is basically buggy, but I was worried to change its behavior without notice since it seemed impossible for people to debug. We should do a (long) deprecation cycle to rename switchPromptlyOnly to switchPromptly and drop the current version of switchPromptly.

[oliver-batchelor]

That is strange, and I agree switchPromptOnly should be the 'normal' version. If the other version did exist it should only be with Monoid or These to highlight the overlap!

[Ericson2314]

I'm curious what affect #371 will have on this.

[JBetz]

@Ericson2314 There's a commented out test case in the RequesterT test suite with a TODO about re-enabling it once this issue has been resolved. The test passes on the #371 branch, so it looks like this will be fixed by the refactor.