bodil / purescript-signal

Elm style FRP library for PureScript
Apache License 2.0
258 stars 44 forks source link

Add `foldpE` function. #59

Closed dmbfm closed 6 years ago

dmbfm commented 7 years ago

When using the foldp function with Eff computations it seems that be some type of memory leak may occur (e.g., https://github.com/michaelficarra/purescript-demo-mario/issues/11), and I was getting "Maximum call stack size exceeded" errors in the "_do" function.

The foldpE function is like the foldp function, but accepts a function of the type a -> b -> Eff e b as an argument, and the action is executed inside the subscribe callback. With this I'm not getting stack problems anymore.

bodil commented 7 years ago

I wonder if this could be generalised to work for any MonadRec, not just Eff?

dmbfm commented 7 years ago

Good Idea, I'm gonna give it a try. At least I should have used MonadEff instead of Eff

dmbfm commented 7 years ago

So, I've been thinking about this for a while but I don't see how to use MonadRec here... since we need to subscribe fot the signal's values in a async fashion, how would we call tailRecM? Have any Ideas, or should we leave it at that?

CarstenKoenig commented 6 years ago

Hey I've had a similar situation where I needed something like this too and I hacked a similar solution (but I think the result should indicate the effects just as any other side-effecting signal function does):

foreign import foldM :: forall a b e. (a -> b -> (Eff e b)) -> b -> (Signal a) -> Eff e (Signal b)

in my case it was not a stack-overflow but I want a RANDOM effect in the update (was for a Tetris game and I wand a new random tetromino ;) ) Now the obvious thing without the function discussed here is using foldp and the Apply-instance to hook up effects - but this results with a huge effect chain that is reevaluated on each signal/runSignal which has the funny effect that the tetrominos are randomized over and over again after the fact that they already fell down ... it's quite a funny effect to be honest :D

so overall: it would be great if you could merge this PR (don't think the failed travis build is really an issue as all tests seemed to have passed)

CarstenKoenig commented 6 years ago

Hello @dmbfm,

if you are still interested in this, then I would love to put this in.

We moved to PureScript 0.12 / Effect so it would be nice if you could modify you PR for this.

Also please change the signature to (see my comment above)

:: forall a b. (a -> b -> Effect b) -> b -> Signal a -> Effect (Signal b)

for bonus points: add a test :smile:

If you don't find the time anymore please let me know - I'll do it for you then.

CarstenKoenig commented 6 years ago

I added a similar version of this PR here https://github.com/bodil/purescript-signal/pull/67 and borrowed your test-case and doc-string for that.

I hope this is ok with you - thank you for your contribution

afcondon commented 5 years ago

Hey, @CarstenKoenig, i know this is long closed but I would love to see an example of how this fix should be used, ie in simple demo like the Mario one that lead to this issue being raised originally.

I have played around with the code a fair bit and can't see how one gets the desired action of Effects being processed but not running the entire process forward from initialisation on every single iteration.

I feel like i should be able to understand it from reading the test/Main but i'm afraid i don't - any help would be much appreciated...