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

Compute code gets inserted into DOM with Stache sometimes #1617

Closed
dylanrtt opened this Issue Apr 10, 2015 · 7 comments

Comments

Projects
None yet
4 participants
@dylanrtt
Contributor

dylanrtt commented Apr 10, 2015

When you render a Stache template with a binding to a key that is not a can.compute, if it changes to a can.compute, the compute's code will be inserted into the DOM. This does not happen in Mustache.

Here's a fiddle.

http://jsfiddle.net/qYdwR/2471/

@daffl daffl added this to the 2.2.5 milestone Apr 10, 2015

@daffl daffl modified the milestones: 2.2.6, 2.2.5 Apr 21, 2015

moschel added a commit that referenced this issue May 2, 2015

@moschel

This comment has been minimized.

Show comment
Hide comment
@moschel

moschel May 2, 2015

Contributor

@dylanrtt added a fix ^

Contributor

moschel commented May 2, 2015

@dylanrtt added a fix ^

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer May 14, 2015

Contributor

@brianmoschel I dont think this is the right fix.

Contributor

justinbmeyer commented May 14, 2015

@brianmoschel I dont think this is the right fix.

@justinbmeyer justinbmeyer self-assigned this May 14, 2015

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer May 14, 2015

Contributor

So, looking into this issue, I think it's worth some discussion.

I'm not sure comptues should be allowed as values of a can.Map. The primary reason is that it will undermine a some of the performance gains in stache because we won't be able to use "single bind" computes.

"single bind" computes are a strategy that if a single map property is being observed, we only listen to just that property change and never look for other observables to listen to.

This speeds up live binding quite a bit. Having a compute as a potential property means we might always need to check for that compute and listen to it.

Btw, this works if you don't initially have an empty property value.

Contributor

justinbmeyer commented May 14, 2015

So, looking into this issue, I think it's worth some discussion.

I'm not sure comptues should be allowed as values of a can.Map. The primary reason is that it will undermine a some of the performance gains in stache because we won't be able to use "single bind" computes.

"single bind" computes are a strategy that if a single map property is being observed, we only listen to just that property change and never look for other observables to listen to.

This speeds up live binding quite a bit. Having a compute as a potential property means we might always need to check for that compute and listen to it.

Btw, this works if you don't initially have an empty property value.

@dylanrtt

This comment has been minimized.

Show comment
Hide comment
@dylanrtt

dylanrtt May 14, 2015

Contributor

I would probably be fine with disallowing computes on maps. Computes have been manually applied to models a lot in the app I work on (some were done before the define plugin), and they recently started showing up as minor problems during the upgrade to stache, and then as very serious problems in the upgrade to can 2.2.1; it took several full days to understand and fix the problems with computes being manually applied to maps.

In almost all cases where computes went wrong in my app, I would say the usage was inappropriate. I was usually able to work around the issue by just rethinking the problem and coming to a more appropriate solution. For example, in one case, a "display" compute was being set on every model in a list, and the compute was depending on the other values in the list. Since it was really performing view logic, and thus did not belong in define, I simply changed it to a local stache helper and fixed the problem.

The only usage I can think of where we needed to apply computes in an unorthodox way had to due with a model list because we can't currently use define on lists. Maybe we could get #1127 expedited sooner...

Contributor

dylanrtt commented May 14, 2015

I would probably be fine with disallowing computes on maps. Computes have been manually applied to models a lot in the app I work on (some were done before the define plugin), and they recently started showing up as minor problems during the upgrade to stache, and then as very serious problems in the upgrade to can 2.2.1; it took several full days to understand and fix the problems with computes being manually applied to maps.

In almost all cases where computes went wrong in my app, I would say the usage was inappropriate. I was usually able to work around the issue by just rethinking the problem and coming to a more appropriate solution. For example, in one case, a "display" compute was being set on every model in a list, and the compute was depending on the other values in the list. Since it was really performing view logic, and thus did not belong in define, I simply changed it to a local stache helper and fixed the problem.

The only usage I can think of where we needed to apply computes in an unorthodox way had to due with a model list because we can't currently use define on lists. Maybe we could get #1127 expedited sooner...

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer May 15, 2015

Contributor

If I were to remove the "single bind" exception, it drops the spinning circle speed from about 12ms per loop to about 16ms per loop.

I suppose I could only setup "single bind" exception on values that were not undefined, or computes.

So, if you switched from undefined to compute, live-binding would be slow, but would work. If you switch from undefined to "foo", live binding will be slow, but work.

If you switched from null to a compute, live binding will not work. Same with "foo", to a compute.

If you switch from null to "foo" live binding will be fast and work.

Alternatively, there might be a way to detect a return of observable values and switch off the exception. This would be ideal, but hard to wire up.

Contributor

justinbmeyer commented May 15, 2015

If I were to remove the "single bind" exception, it drops the spinning circle speed from about 12ms per loop to about 16ms per loop.

I suppose I could only setup "single bind" exception on values that were not undefined, or computes.

So, if you switched from undefined to compute, live-binding would be slow, but would work. If you switch from undefined to "foo", live binding will be slow, but work.

If you switched from null to a compute, live binding will not work. Same with "foo", to a compute.

If you switch from null to "foo" live binding will be fast and work.

Alternatively, there might be a way to detect a return of observable values and switch off the exception. This would be ideal, but hard to wire up.

@dylanrtt

This comment has been minimized.

Show comment
Hide comment
@dylanrtt

dylanrtt May 15, 2015

Contributor

Either option sounds fine, but at this point I don't really see this as much of an issue anymore. I was able to work around it fairly easily and I just figured you guys should be aware of it since it seemed like a regression. Up to you how you want to deal with it, whether that be a fix or just documentation.

Contributor

dylanrtt commented May 15, 2015

Either option sounds fine, but at this point I don't really see this as much of an issue anymore. I was able to work around it fairly easily and I just figured you guys should be aware of it since it seemed like a regression. Up to you how you want to deal with it, whether that be a fix or just documentation.

justinbmeyer added a commit that referenced this issue May 16, 2015

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer May 16, 2015

Contributor

@dylanrtt I was able to "have our cake and eat it" with an improved compute that is able to switch between the "single bind" and the "multi bind". This way we can have good performance, but if suddenly a compute is added, can.stache and actually can.mustache will handle it.

Contributor

justinbmeyer commented May 16, 2015

@dylanrtt I was able to "have our cake and eat it" with an improved compute that is able to switch between the "single bind" and the "multi bind". This way we can have good performance, but if suddenly a compute is added, can.stache and actually can.mustache will handle it.

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