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

Time.delay (Native.Signal.delay) has no kids #295

Closed
wetmore opened this Issue Jul 14, 2015 · 7 comments

Comments

Projects
None yet
4 participants
@wetmore

wetmore commented Jul 14, 2015

The delay function creates a node in the signal graph with no kids. This means any module using delay cannot be opened in the reactor debugger, because the following code in debug.js will throw an error:

function addAllToDict(node)
    {
        nodesById[node.id] = node;
        node.kids.forEach(addAllToDict);
    }
    nodes.forEach(addAllToDict);

node.kids is undefined for an output-delay-output-n node (where n is the delay in millis).

I don't know much about the signal implementation so I don't know if the lack of kids is intentional, but if it is the debugger code should account for this. Otherwise it's impossible to use the debugger on code with delay which is a bummer.

@AlexNisnevich

This comment has been minimized.

Show comment
Hide comment
@AlexNisnevich

AlexNisnevich Aug 1, 2015

Contributor

Oh interesting. I had been wondering why the debugger stopped working for https://github.com/AlexNisnevich/kalevala after I started using delay.

I second that the debugger should account for this somehow. Or at least it should be documented somewhere that using delay breaks the debugger.

Contributor

AlexNisnevich commented Aug 1, 2015

Oh interesting. I had been wondering why the debugger stopped working for https://github.com/AlexNisnevich/kalevala after I started using delay.

I second that the debugger should account for this somehow. Or at least it should be documented somewhere that using delay breaks the debugger.

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Aug 2, 2015

Member

Thanks for the report! I think it may make sense to move this to the elm-reactor repo. cc @vilterp to confirm or deny :)

Member

evancz commented Aug 2, 2015

Thanks for the report! I think it may make sense to move this to the elm-reactor repo. cc @vilterp to confirm or deny :)

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Aug 2, 2015

Member

I am trying to be more intense about closing issues. I think the resolution here is "move the issue to this repo" so I'm just gonna commit to it. Thanks again for reporting this!

Member

evancz commented Aug 2, 2015

I am trying to be more intense about closing issues. I think the resolution here is "move the issue to this repo" so I'm just gonna commit to it. Thanks again for reporting this!

@evancz evancz closed this Aug 2, 2015

@vilterp

This comment has been minimized.

Show comment
Hide comment
@vilterp

vilterp Aug 2, 2015

Signal.delay creates an input and an output; I think if the output gets registered as a kid of the given node the reactor will be fine. I'll submit a PR; it should be like a one line change.

vilterp commented Aug 2, 2015

Signal.delay creates an input and an output; I think if the output gets registered as a kid of the given node the reactor will be fine. I'll submit a PR; it should be like a one line change.

@wetmore

This comment has been minimized.

Show comment
Hide comment
@wetmore

wetmore Aug 3, 2015

I didn't see your reply Pete and posted this.

Feel free to close it if you'd like.

wetmore commented Aug 3, 2015

I didn't see your reply Pete and posted this.

Feel free to close it if you'd like.

@vilterp

This comment has been minimized.

Show comment
Hide comment
@vilterp

vilterp Aug 3, 2015

Replied there, thanks!

Re: my earlier comment: this was already happening; the problem was that the new output node had no children. This is accounted for in my new reactor code.

vilterp commented Aug 3, 2015

Replied there, thanks!

Re: my earlier comment: this was already happening; the problem was that the new output node had no children. This is accounted for in my new reactor code.

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Aug 3, 2015

Member

Thanks for working through this together! :D

Member

evancz commented Aug 3, 2015

Thanks for working through this together! :D

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