New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't make timeoutId in fpsWhen part of the state stored in signal graph. #160

Merged
merged 1 commit into from Feb 6, 2015

Conversation

Projects
None yet
2 participants
@jvoigtlaender
Contributor

jvoigtlaender commented Feb 6, 2015

This question didn't get an answer. But I think we don't actually want timeoutId to be part of the state that is copied over from the old signal graph to the new one on hotswap, for otherwise (in certain constellations where a hotswap occurs while a timeout is in flight on the old graph and the initialization of the new graph sets a timeout as well) we might sometimes cancel the wrong timeout (namely one affecting the old instead of the new signal graph).

Hence, the proposal to turn timeoutId back into a local variable.

subtlety in fpsWhen
We don't actually want `timeoutId` to be part of the state that is
copied over from the old signal graph to the new one on hotswap, for
otherwise (in certain constellations where a hotswap occurs while a
timeout is in flight on the old graph and the initialization of the new
graph sets a timeout as well) we might sometimes cancel the wrong
timeout (namely one affecting the old instead of the new signal graph).
@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Feb 6, 2015

Member

Looks good to me! Let's merge, but have you been able to test it out in the different ways?

Member

evancz commented Feb 6, 2015

Looks good to me! Let's merge, but have you been able to test it out in the different ways?

evancz pushed a commit that referenced this pull request Feb 6, 2015

Merge pull request #160 from jvoigtlaender/timeoutId-fpsWhen
Don't make timeoutId in fpsWhen part of the state stored in signal graph.

@evancz evancz merged commit 29acc5f into elm:master Feb 6, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Feb 6, 2015

Contributor

I could not test/verify the impact of this change by actually checking it running with hotswapping in the Reactor before/after. The reason for this is (or seems to be) what I asked about at another place: When having locally modified the runtime, running a test program in "normal mode" will indeed use that modified runtime, but running a test program in "debugger mode" (using the wrench symbol) appears to use an unmodified runtime that was installed at the time Elm was installed. (Apparently, it uses a version of the runtime that was compiled into debug.js that sits in a subdirectory of where the Elm platform lives.)

So, for this commit here, my statements about how hotswapping interacts with whether or not timeoutId is made part of the signal graph state, are just by thinking about the code and the implementation of hotswapping in Runtime.js, not by experimentation.

Contributor

jvoigtlaender commented Feb 6, 2015

I could not test/verify the impact of this change by actually checking it running with hotswapping in the Reactor before/after. The reason for this is (or seems to be) what I asked about at another place: When having locally modified the runtime, running a test program in "normal mode" will indeed use that modified runtime, but running a test program in "debugger mode" (using the wrench symbol) appears to use an unmodified runtime that was installed at the time Elm was installed. (Apparently, it uses a version of the runtime that was compiled into debug.js that sits in a subdirectory of where the Elm platform lives.)

So, for this commit here, my statements about how hotswapping interacts with whether or not timeoutId is made part of the signal graph state, are just by thinking about the code and the implementation of hotswapping in Runtime.js, not by experimentation.

@jvoigtlaender jvoigtlaender deleted the jvoigtlaender:timeoutId-fpsWhen branch Feb 6, 2015

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Feb 6, 2015

Member

It's because the sidebar is written in elm, and everything it needs is bundled. So there can be clashes (I have run into this) and we have a fix in mind. You can maybe get around this by building from source, but I'm going to trust that this better. Let's try to remember to check though!

Member

evancz commented Feb 6, 2015

It's because the sidebar is written in elm, and everything it needs is bundled. So there can be clashes (I have run into this) and we have a fix in mind. You can maybe get around this by building from source, but I'm going to trust that this better. Let's try to remember to check though!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment