Replace "constant" by "input" where appropriate

[jvoigtlaender]

No description provided.

[jvoigtlaender]

Like this?

[jvoigtlaender]

Like this?

[jvoigtlaender]

Corresponding changes are still to be done in Http.js, Keyboard.js, and WebSockets.js, but due to expected merge conflicts it does not make sense to go at these before https://github.com/elm-lang/core/pull/127 has been integrated into master.

[jvoigtlaender]

Corresponding changes are still to be done in Http.js, Keyboard.js, and WebSockets.js, but due to expected merge conflicts it does not make sense to go at these before https://github.com/elm-lang/core/pull/127 has been integrated into master.

[jvoigtlaender]

That should be it.

There are still calls to Signal.constant from JS here: https://github.com/jvoigtlaender/core/blob/constant-to-input/src/Native/Runtime.js#L289 and here: https://github.com/jvoigtlaender/core/blob/constant-to-input/src/Native/Time.js#L59. But at least the latter is really also "semantically" meant to be a constant signal. I'm not sure about the former occurrence in Runtime.js.

[jvoigtlaender]

That should be it.

There are still calls to Signal.constant from JS here: https://github.com/jvoigtlaender/core/blob/constant-to-input/src/Native/Runtime.js#L289 and here: https://github.com/jvoigtlaender/core/blob/constant-to-input/src/Native/Time.js#L59. But at least the latter is really also "semantically" meant to be a constant signal. I'm not sure about the former occurrence in Runtime.js.

[jvoigtlaender]

Actually, the occurrence of Signal.constant at https://github.com/jvoigtlaender/core/blob/constant-to-input/src/Native/Runtime.js#L289 should probably be replaced by Native.Signal.input, as well as the occurrence of Signal.constant here: https://github.com/elm-lang/elm-reactor/blob/60b8dca16d5ba4a95a6a2aafe7d89a5a207639d6/frontend/debugger-implementation.js#L729.

[jvoigtlaender]

Actually, the occurrence of Signal.constant at https://github.com/jvoigtlaender/core/blob/constant-to-input/src/Native/Runtime.js#L289 should probably be replaced by Native.Signal.input, as well as the occurrence of Signal.constant here: https://github.com/elm-lang/elm-reactor/blob/60b8dca16d5ba4a95a6a2aafe7d89a5a207639d6/frontend/debugger-implementation.js#L729.

[evancz]

Yeah, I think that's fine. It feels a little weird to be importing both Signal and Native.Signal though, that's my one hesitation. Do you have an opinion about whether what the right thing is here?

[evancz]

Yeah, I think that's fine. It feels a little weird to be importing both Signal and Native.Signal though, that's my one hesitation. Do you have an opinion about whether what the right thing is here?

[evancz]

Oops, I commented but had not refreshed the page, so I did not see any of the comments from today! I'll review again now!

[evancz]

Oops, I commented but had not refreshed the page, so I did not see any of the comments from today! I'll review again now!

[evancz]

Everything looks good. I'd be sort of inclined to use Native.Signal for all of the map needs, but I'm not sure if that's a good idea or if it really matters.

[evancz]

Everything looks good. I'd be sort of inclined to use Native.Signal for all of the map needs, but I'm not sure if that's a good idea or if it really matters.

[jvoigtlaender]

Oh, right, I hadn't noticed that most of the remaining Signal use is for Signal.map, and Signal.map is just an indirection to Native.Signal.map. So yes, one could avoid a bunch of indirections here. I'll not touch this, though.

But I'll do PRs to replace the constant uses at https://github.com/elm-lang/core/blob/master/src/Native/Runtime.js#L289 and https://github.com/elm-lang/elm-reactor/blob/60b8dca16d5ba4a95a6a2aafe7d89a5a207639d6/frontend/debugger-implementation.js#L729, because these are signals that will get updates.

[jvoigtlaender]

Oh, right, I hadn't noticed that most of the remaining Signal use is for Signal.map, and Signal.map is just an indirection to Native.Signal.map. So yes, one could avoid a bunch of indirections here. I'll not touch this, though.

But I'll do PRs to replace the constant uses at https://github.com/elm-lang/core/blob/master/src/Native/Runtime.js#L289 and https://github.com/elm-lang/elm-reactor/blob/60b8dca16d5ba4a95a6a2aafe7d89a5a207639d6/frontend/debugger-implementation.js#L729, because these are signals that will get updates.

[jvoigtlaender]

I take these back. In those places, when those signals are created, they are actually constant. (Because it's for cases like where main : Element is artifically made a main : Signal Element.)

[jvoigtlaender]

I take these back. In those places, when those signals are created, they are actually constant. (Because it's for cases like where main : Element is artifically made a main : Signal Element.)

[evancz]

Yeah, I agree, constant makes sense in those cases. We'll save map for another time, when we are feeling more decisive :)

Thanks a ton for improving this!

[evancz]

Yeah, I agree, constant makes sense in those cases. We'll save map for another time, when we are feeling more decisive :)

Thanks a ton for improving this!