Skip to content
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

[WIP] Attempting to fix nested signals #191

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

izaid
Copy link
Contributor

@izaid izaid commented Oct 3, 2016

This is a PR that attempts to fix the issue of broken nested signals, see #190. Here is what I have found so far:

  1. The Patchwork diff generated in start_updates comes from comparing against Escher.empty. Check out https://github.com/shashi/Escher.jl/blob/master/src/cli/serve.jl#L119. This is not right, the right thing to do is compare against the last rendered tile. I've made this work in the PR with a cache of sorts, and using that gives the correct Patchwork diff.

  2. With 1) in place, I'm still having issues on the JavaScript side with not finding the right node to apply the patch to. Not sure why this is the case, but it may be related to 3).

  3. I'm not sure if this is a problem or not, but a new nested signal appears to be created in each iteration of the outer signal. This means the signal ID constantly changes. While Patchwork patches this appropriately, it seems inefficient and means we can't memoize on signal IDs.

@shashi
Copy link
Member

shashi commented Oct 3, 2016

  1. The Patchwork diff generated in start_updates comes from comparing against Escher.empty. Check out https://github.com/shashi/Escher.jl/blob/master/src/cli/serve.jl#L119. This is not right, the right thing to do is compare against the last rendered tile

Escher.empty is the initial value rendered (<div></div>) on the browser as well. So the first update should be a patch against that, right?

  1. is right. nested maps are inefficient, it's great if you have figured out a way to make it efficient.

I'll go over this in more detail soon. Thanks!

@izaid
Copy link
Contributor Author

izaid commented Oct 3, 2016

Yeah, Esher.empty has to be the initial value. But the way it is currently set up, we're actually diffing against that in subsequent updates for the inner signals (which is the basic problem).

With regard to 3), where is the new Signal being constructed? I tried to hunt this down, but failed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants