Fix semantics of fpsWhen.

[jwmerrill]

fixes #139
alternative to #141

Semantics:

1. fpsWhen fires whenever isOn changes, or its internal setTimeout fires
2. Its value is a timedelta, which is 0 when isOn changes from false to true,
   or is the time elapsed since the last event otherwise. As a corrolary, summing
   deltas gives exactly the time between when isOn transitioned from false to
   true, and when it transitioned from true to false.

[jvoigtlaender]

Looks okay also, except:

When the isOn signal passed to fpsWhen is initially False, you call clearTimeout on the uninitialized timeoutId variable.

[jvoigtlaender]

Looks okay also, except:

When the isOn signal passed to fpsWhen is initially False, you call clearTimeout on the uninitialized timeoutId variable.

[jwmerrill]

When the isOn signal passed to fpsWhen is initially False, you call clearTimeout on the uninitialized timeoutId variable.

I could buy an argument that this is bad style, but it doesn't cause any problems.

Per the spec

If handle does not identify an entry in the list of active timers of the WindowTimers object on which the method was invoked, the method does nothing.

and it seems to be implemented this way in all the major browsers.

[jwmerrill]

When the isOn signal passed to fpsWhen is initially False, you call clearTimeout on the uninitialized timeoutId variable.

I could buy an argument that this is bad style, but it doesn't cause any problems.

Per the spec

If handle does not identify an entry in the list of active timers of the WindowTimers object on which the method was invoked, the method does nothing.

and it seems to be implemented this way in all the major browsers.

[jvoigtlaender]

Well, okay. But wouldn't adding an if (wasOn) on line https://github.com/jwmerrill/core/blob/alternate-fpswhen-fix/src/Native/Time.js#L41 still be nicer than relying on the fact that clearTimeout(undefined) should do no harm?

[jvoigtlaender]

Well, okay. But wouldn't adding an if (wasOn) on line https://github.com/jwmerrill/core/blob/alternate-fpswhen-fix/src/Native/Time.js#L41 still be nicer than relying on the fact that clearTimeout(undefined) should do no harm?

[jvoigtlaender]

BTW, it might also be a bit nicer (same in my alternative PR), to have var wasOn = false; in this line: https://github.com/jwmerrill/core/blob/alternate-fpswhen-fix/src/Native/Time.js#L29. It would be more obvious then that if the initial value of the isOn signal is True, it gets "turned on" at initialization time of the program. (Semantics is unchanged, since in the situation where the proposed change has an impact, timestamp - previousTime in line https://github.com/jwmerrill/core/blob/alternate-fpswhen-fix/src/Native/Time.js#L46 will be 0 anyway.)

[jvoigtlaender]

BTW, it might also be a bit nicer (same in my alternative PR), to have var wasOn = false; in this line: https://github.com/jwmerrill/core/blob/alternate-fpswhen-fix/src/Native/Time.js#L29. It would be more obvious then that if the initial value of the isOn signal is True, it gets "turned on" at initialization time of the program. (Semantics is unchanged, since in the situation where the proposed change has an impact, timestamp - previousTime in line https://github.com/jwmerrill/core/blob/alternate-fpswhen-fix/src/Native/Time.js#L46 will be 0 anyway.)

[jwmerrill]

wouldn't adding an if (wasOn) on line https://github.com/jwmerrill/core/blob/alternate-fpswhen-fix/src/Native/Time.js#L41 still be nicer than relying on the fact that clearTimeout(undefined) should do no harm?

Yes, done.

[jwmerrill]

wouldn't adding an if (wasOn) on line https://github.com/jwmerrill/core/blob/alternate-fpswhen-fix/src/Native/Time.js#L41 still be nicer than relying on the fact that clearTimeout(undefined) should do no harm?

Yes, done.

[jvoigtlaender]

I've closed https://github.com/elm-lang/core/pull/141 in favour of this PR here.

[jvoigtlaender]

I've closed https://github.com/elm-lang/core/pull/141 in favour of this PR here.

[jwmerrill]

it might also be a bit nicer (same in my alternative PR), to have var wasOn = false

Agreed--makes the semantics for the first value clearer. Done.

[jwmerrill]

it might also be a bit nicer (same in my alternative PR), to have var wasOn = false

Agreed--makes the semantics for the first value clearer. Done.

[jwmerrill]

Incidentally, if I was implementing this in Elm instead of JS, I think it would be better to use Signal.merge instead of Signal.map2 to create the main input here: https://github.com/elm-lang/core/blob/b1891ed42cd083761195abafaa4e287333ecf04d/src/Native/Time.js#L27

The merged type could be something like

type Action = Toggle Bool | Tick

and then inside update we could match on the input type.

But in JS, it seems like this would make things less clear instead of more.

[jwmerrill]

Incidentally, if I was implementing this in Elm instead of JS, I think it would be better to use Signal.merge instead of Signal.map2 to create the main input here: https://github.com/elm-lang/core/blob/b1891ed42cd083761195abafaa4e287333ecf04d/src/Native/Time.js#L27

The merged type could be something like

type Action = Toggle Bool | Tick

and then inside update we could match on the input type.

But in JS, it seems like this would make things less clear instead of more.

[jvoigtlaender]

This whole PR fixes the semantics of fpsWhen in "normal execution mode". In the presence of hot-swapping, the signal still has problems. Hot-swapping modules in general only works reliably if every state relevant for a signal is part of a signal's .value (might be the same signal or a signal from which it is derived), because such values are the only kind of information that is copied over from the old signal graph to the new signal graph on hot-swapping. Here now the wasOn, previousTime and timeoutId are such relevant state, but not part of any signal's .value. That problem was not actually introduced here, or by my earlier changes in https://github.com/elm-lang/core/pull/76 or refactorings since then. Looking back through the history of the Time.js file, it has existed for a long while, since the various variables like wasOn, prev, timeoutID were not attached to a signal then either.

Likely solution: introduce a foldp somehow to manage handling of wasOn, previousTime, timeoutId (should be easier now than in the earlier version, where the main update, then called startStopTimer, was mapped over two signals).

[jvoigtlaender]

This whole PR fixes the semantics of fpsWhen in "normal execution mode". In the presence of hot-swapping, the signal still has problems. Hot-swapping modules in general only works reliably if every state relevant for a signal is part of a signal's .value (might be the same signal or a signal from which it is derived), because such values are the only kind of information that is copied over from the old signal graph to the new signal graph on hot-swapping. Here now the wasOn, previousTime and timeoutId are such relevant state, but not part of any signal's .value. That problem was not actually introduced here, or by my earlier changes in https://github.com/elm-lang/core/pull/76 or refactorings since then. Looking back through the history of the Time.js file, it has existed for a long while, since the various variables like wasOn, prev, timeoutID were not attached to a signal then either.

Likely solution: introduce a foldp somehow to manage handling of wasOn, previousTime, timeoutId (should be easier now than in the earlier version, where the main update, then called startStopTimer, was mapped over two signals).

[evancz]

Is it possible to figure out how to write some tests for this? @laszlopandy is concerned that the semantics are easy to change accidentally, and it's easy for people to rely on certain oddities or be impacted by changes.

[evancz]

Is it possible to figure out how to write some tests for this? @laszlopandy is concerned that the semantics are easy to change accidentally, and it's easy for people to rely on certain oddities or be impacted by changes.

[evancz]

I don't think we need a general purpose signal testing framework, we just need a way to verify that the core signal functions work as we expect. It seems that this can be done entirely in JS by just dealing with native modules.

[evancz]

I don't think we need a general purpose signal testing framework, we just need a way to verify that the core signal functions work as we expect. It seems that this can be done entirely in JS by just dealing with native modules.

[jvoigtlaender]

I'm afraid I don't have anything to contribute to a principled way of testing signal implementations. I've been doing this by hand using Elm code like shown in #139.

About the specific case of fpsWhen:

• I've now written a version that packages all state in a signal, which should make hotswapping work okay: #143.
• The semantics certainly changes here from what it was in Elm since at least 0.13. But that semantics was clearly in direct and strong conflict with what the documentation of fpsWhen is saying. Events that shouldn't be there. Time deltas not summing up to what they should. See the first example in #139. So if in this case people have been relying on the oddities, their code should really be updated. I don't see a point of even trying to be backwards compatible here.

[jvoigtlaender]

I'm afraid I don't have anything to contribute to a principled way of testing signal implementations. I've been doing this by hand using Elm code like shown in #139.

About the specific case of fpsWhen:

• I've now written a version that packages all state in a signal, which should make hotswapping work okay: #143.
• The semantics certainly changes here from what it was in Elm since at least 0.13. But that semantics was clearly in direct and strong conflict with what the documentation of fpsWhen is saying. Events that shouldn't be there. Time deltas not summing up to what they should. See the first example in #139. So if in this case people have been relying on the oddities, their code should really be updated. I don't see a point of even trying to be backwards compatible here.

[jwmerrill]

Closing in favor of #143

[jwmerrill]

Closing in favor of #143