added $index value in the binding context of foreach templates. #182

Merged
merged 14 commits into from Mar 23, 2012

Conversation

Projects
None yet
9 participants
@mbest
Member

mbest commented Feb 23, 2012

There is probably a much better way to accomplish this. Code review required

@rniemeyer

This comment has been minimized.

Show comment
Hide comment
@rniemeyer

rniemeyer Oct 22, 2011

Member

I had been thinking a little about $index and I believe that it would be useful to make $index an observable.

Currently, your method adds $index, but does not adjust it when items are shifted. So, if it was adjusted and if $index was an observable, then if a binding does use it, it would get re-evaluated on a change.

Additionally, when I tried your changes, I had to add an index parameter to activateBindingsCallback and pass it to the call to createInnerBindingContext in that function.

Member

rniemeyer commented Oct 22, 2011

I had been thinking a little about $index and I believe that it would be useful to make $index an observable.

Currently, your method adds $index, but does not adjust it when items are shifted. So, if it was adjusted and if $index was an observable, then if a binding does use it, it would get re-evaluated on a change.

Additionally, when I tried your changes, I had to add an index parameter to activateBindingsCallback and pass it to the call to createInnerBindingContext in that function.

@barkmadley

This comment has been minimized.

Show comment
Hide comment
@barkmadley

barkmadley Oct 22, 2011

Contributor

You are probably right. I will need to think a bit harder about how to make that work (and some more tests).

The test I added works without your change, but that could be because the dummyTemplateEngine is implemented slightly differently to a normal template engine (I found some funny behaviour in the memoization functionality, took me a while of stepping through the code to see what was going on). I updated the branch to reflect the added parameter.

Contributor

barkmadley commented Oct 22, 2011

You are probably right. I will need to think a bit harder about how to make that work (and some more tests).

The test I added works without your change, but that could be because the dummyTemplateEngine is implemented slightly differently to a normal template engine (I found some funny behaviour in the memoization functionality, took me a while of stepping through the code to see what was going on). I updated the branch to reflect the added parameter.

@barkmadley

This comment has been minimized.

Show comment
Hide comment
@barkmadley

barkmadley Oct 22, 2011

Contributor

I tried to get observable indices working, however I am still getting a bug (the new test case fails on the 3rd assertion) that I can't seem to figure out. Can you take a look at it ryan?

Contributor

barkmadley commented Oct 22, 2011

I tried to get observable indices working, however I am still getting a bug (the new test case fails on the 3rd assertion) that I can't seem to figure out. Can you take a look at it ryan?

@rniemeyer

This comment has been minimized.

Show comment
Hide comment
@rniemeyer

rniemeyer Oct 23, 2011

Member

Will try to help with this one, as soon as I get a chance.

I kind of have mixed feelings on this functionality though. In most cases, it is not needed. I wonder if this is too much work/complexity/overhead, when people can easily attach an index to their observableArray using the subscribe technique that I usually recommend and in a dependentObservable (filtered array) they can attach an index themselves. There are downsides to that way though (adds properties to your items, doesn't work on arrays of primitives).

I suppose there could be an option to turn index tracking on/off in the template binding.

not sure if Steve can think of an easier way to provide this functionality

Member

rniemeyer commented Oct 23, 2011

Will try to help with this one, as soon as I get a chance.

I kind of have mixed feelings on this functionality though. In most cases, it is not needed. I wonder if this is too much work/complexity/overhead, when people can easily attach an index to their observableArray using the subscribe technique that I usually recommend and in a dependentObservable (filtered array) they can attach an index themselves. There are downsides to that way though (adds properties to your items, doesn't work on arrays of primitives).

I suppose there could be an option to turn index tracking on/off in the template binding.

not sure if Steve can think of an easier way to provide this functionality

@barkmadley

This comment has been minimized.

Show comment
Hide comment
@barkmadley

barkmadley Oct 24, 2011

Contributor

I think the main problem is that native templates are perceived as a replacement for jQuery templates. Since jQuery templates support an index parameter in their looping construct, not having one makes it appear as if native templates are incomplete in comparison. Undoubtedly this was probably a feature that some users used so they will likely complain. We are already starting to see this coming up on the mailing list more often.

Contributor

barkmadley commented Oct 24, 2011

I think the main problem is that native templates are perceived as a replacement for jQuery templates. Since jQuery templates support an index parameter in their looping construct, not having one makes it appear as if native templates are incomplete in comparison. Undoubtedly this was probably a feature that some users used so they will likely complain. We are already starting to see this coming up on the mailing list more often.

@barkmadley

This comment has been minimized.

Show comment
Hide comment
@barkmadley

barkmadley Oct 24, 2011

Contributor

The most recent commit fixes the logic error that was in the patch. However the testsuite still does not work.

After a lot more debugging I got fed up with the testsuite and tried to use the code I had already written in a simple test case. Funnily enough it does work with the native template engine. The problem appears to be a disparity between the native template engine and the dummyTemplateEngine used in the testsuite.

Contributor

barkmadley commented Oct 24, 2011

The most recent commit fixes the logic error that was in the patch. However the testsuite still does not work.

After a lot more debugging I got fed up with the testsuite and tried to use the code I had already written in a simple test case. Funnily enough it does work with the native template engine. The problem appears to be a disparity between the native template engine and the dummyTemplateEngine used in the testsuite.

@barkmadley

This comment has been minimized.

Show comment
Hide comment
@barkmadley

barkmadley Oct 24, 2011

Contributor

most notably using strings internally causes the "nodes" of the dummyTemplateEngine to be immutable, whereas with real template engines that work with nodes, the children are mutable. We can either fix the dummyTemplateEngine to use something other than strings internally or we can change the $index tactic such that when "retaining" nodes, it will perform a callback that can update sub-nodes.

Contributor

barkmadley commented Oct 24, 2011

most notably using strings internally causes the "nodes" of the dummyTemplateEngine to be immutable, whereas with real template engines that work with nodes, the children are mutable. We can either fix the dummyTemplateEngine to use something other than strings internally or we can change the $index tactic such that when "retaining" nodes, it will perform a callback that can update sub-nodes.

@rniemeyer

This comment has been minimized.

Show comment
Hide comment
@rniemeyer

rniemeyer Oct 24, 2011

Member

Glad that you got it working. It does seem like a feature that many people want. Will be interested to see what Steve thinks about this approach.

Member

rniemeyer commented Oct 24, 2011

Glad that you got it working. It does seem like a feature that many people want. Will be interested to see what Steve thinks about this approach.

finally cracked it. the problem was duplicate context creation.
it is also necessary to perform double buffering of the stored contexts such
that we do not overwrite new nodes or move new nodes by mistake.
@barkmadley

This comment has been minimized.

Show comment
Hide comment
@barkmadley

barkmadley Oct 24, 2011

Contributor

So I found the problem. There are two calls to createInnerBindingContext in ko.renderTemplateForEach which is where I was creating the $index observable.

Deduplicating this with a contexts buffer that can then be updated with new contexts and moved contexts via double buffering solves all the problems with the test suite. Give it a go.

Contributor

barkmadley commented Oct 24, 2011

So I found the problem. There are two calls to createInnerBindingContext in ko.renderTemplateForEach which is where I was creating the $index observable.

Deduplicating this with a contexts buffer that can then be updated with new contexts and moved contexts via double buffering solves all the problems with the test suite. Give it a go.

@SteveSanderson

This comment has been minimized.

Show comment
Hide comment
@SteveSanderson

SteveSanderson Oct 24, 2011

Contributor

Wow, excellent. Nice work with this.

I haven't yet looked through the implementation in detail, but in principle I agree it would be a useful feature to add.

Is it OK if I schedule this for inclusion in 1.3.1? I'm trying to avoid all new features for 1.3.0 now so that the release can be completed in a finite amount of time :)

Contributor

SteveSanderson commented Oct 24, 2011

Wow, excellent. Nice work with this.

I haven't yet looked through the implementation in detail, but in principle I agree it would be a useful feature to add.

Is it OK if I schedule this for inclusion in 1.3.1? I'm trying to avoid all new features for 1.3.0 now so that the release can be completed in a finite amount of time :)

@barkmadley

This comment has been minimized.

Show comment
Hide comment
@barkmadley

barkmadley Oct 24, 2011

Contributor

1.3.1 is fine. I am surprised this update isn't being called 2.0 considering the amount of work going into it.

Contributor

barkmadley commented Oct 24, 2011

1.3.1 is fine. I am surprised this update isn't being called 2.0 considering the amount of work going into it.

@sibartlett

This comment has been minimized.

Show comment
Hide comment
@sibartlett

sibartlett Oct 25, 2011

Contributor

If semantic versioning were used it would be a 2.0 release, as this version of Knockout has breaking changes (particularly in regards to template engines, and the requirement on latest version of jquery templates).

Contributor

sibartlett commented Oct 25, 2011

If semantic versioning were used it would be a 2.0 release, as this version of Knockout has breaking changes (particularly in regards to template engines, and the requirement on latest version of jquery templates).

Merge branch 'master' into foreach-context-index
* master:
  More indentation tweaks for tidiness
  Update the build
  Minor indentation tweaks
  Stylistic tweaks to previous commit
  Eliminate redundant IE6/7 workarounds for radio/checkbox issues. These problems no longer apply now that bindings are always applied to elements after they've been put into the DOM. Fixes issue #169
  Refactor bad code
  minor fix
  Fix render array with \"undefined\" and \"null\" items in \"foreach\"  template
  Updated remove and removeAll to modify their underlying arrays rather than creating new arrays. This makes them consistent with the rest of the array write functions. Added tests to verify original array is modified. Also added test to verify there is no notification when nothing is removed.
  changing IE detection to not rely on user agents
  Fixed a variable which should not have been global.
@pilavdzic

This comment has been minimized.

Show comment
Hide comment
@pilavdzic

pilavdzic Oct 31, 2011

I am looking forward to 1.3.1 where this feature will be added.

It is very useful in places where the order of your list matters, for example.

Also, if you are working with legacy jquery templates that called custom functions that used index and you are now upgrading to ko built in templates but don't want to re-write a lot of code so you don't introduce bugs ;)

Finally, when there are nested arrays, it is lots of extra work to attach dependantobservables to each child array every time a new one is created to just implement this yourself.

I think it's GREAT idea to include in knockout.. it has been requested by many people. Sure there are other ways of doing it, but they are all much more complex and this makes things much easier for newer folks.

Thanks to everyone for this patch. I am glad it's coming and can't wait... Until then I have to write all this extra code now to workaround the issues I have currently.

I am looking forward to 1.3.1 where this feature will be added.

It is very useful in places where the order of your list matters, for example.

Also, if you are working with legacy jquery templates that called custom functions that used index and you are now upgrading to ko built in templates but don't want to re-write a lot of code so you don't introduce bugs ;)

Finally, when there are nested arrays, it is lots of extra work to attach dependantobservables to each child array every time a new one is created to just implement this yourself.

I think it's GREAT idea to include in knockout.. it has been requested by many people. Sure there are other ways of doing it, but they are all much more complex and this makes things much easier for newer folks.

Thanks to everyone for this patch. I am glad it's coming and can't wait... Until then I have to write all this extra code now to workaround the issues I have currently.

@barkmadley

This comment has been minimized.

Show comment
Hide comment
@barkmadley

barkmadley Nov 1, 2011

Contributor

one of the limitations of the current patch is if you have nested foreach templates then it will be difficult to create a reference to the outer $index from the inner foreach. (jQuery templates didn't have this problem because you bound a custom name to the index in the loop). I might put some effort into adding a parameter to the template options that allow you to specify a variable name for this use case.

Contributor

barkmadley commented Nov 1, 2011

one of the limitations of the current patch is if you have nested foreach templates then it will be difficult to create a reference to the outer $index from the inner foreach. (jQuery templates didn't have this problem because you bound a custom name to the index in the loop). I might put some effort into adding a parameter to the template options that allow you to specify a variable name for this use case.

@while0pass

This comment has been minimized.

Show comment
Hide comment
@while0pass

while0pass Nov 15, 2011

Besides $index variable in templates it would be also great to have variables like $isLast and $isFirst. It is also usefull to have a reference to the parent loop. In django templates all those variables are composed within a namespace "forloop" that is accessible within every for loop as a variable:

https://docs.djangoproject.com/en/dev/ref/templates/builtins/#for

I suppose all those variables are rather handy to have: 4 versions of index counter (0-based, 1-based, and two reversed ones), first and last items booleans and parent loop reference. for instance:

$forloop.index0
$forloop.index1
$forloop.revIndex0
$forloop.revIndex1
$forloop.isLast
$forloop.isFirst
$forloop.parentLoop

or smth like this.

Besides $index variable in templates it would be also great to have variables like $isLast and $isFirst. It is also usefull to have a reference to the parent loop. In django templates all those variables are composed within a namespace "forloop" that is accessible within every for loop as a variable:

https://docs.djangoproject.com/en/dev/ref/templates/builtins/#for

I suppose all those variables are rather handy to have: 4 versions of index counter (0-based, 1-based, and two reversed ones), first and last items booleans and parent loop reference. for instance:

$forloop.index0
$forloop.index1
$forloop.revIndex0
$forloop.revIndex1
$forloop.isLast
$forloop.isFirst
$forloop.parentLoop

or smth like this.

@barkmadley

This comment has been minimized.

Show comment
Hide comment
@barkmadley

barkmadley Nov 16, 2011

Contributor

I did wonder why the binding context has the parent data item as the $parent context variable, but the parent binding context isn't available (this would essentially give you the parent loop or with or other context creating template binding).

Every variable listed there (except for the 0-index and the parent context) is computable from the given list and the current index so I don't see a reason to add them to the framework merely for convenience. Perhaps as some utility functions to make it less verbose. Really I just feel that pre-computing these values isn't worth it if 99% of the time they aren't going to be used.

Contributor

barkmadley commented Nov 16, 2011

I did wonder why the binding context has the parent data item as the $parent context variable, but the parent binding context isn't available (this would essentially give you the parent loop or with or other context creating template binding).

Every variable listed there (except for the 0-index and the parent context) is computable from the given list and the current index so I don't see a reason to add them to the framework merely for convenience. Perhaps as some utility functions to make it less verbose. Really I just feel that pre-computing these values isn't worth it if 99% of the time they aren't going to be used.

@while0pass

This comment has been minimized.

Show comment
Hide comment
@while0pass

while0pass Nov 20, 2011

Every variable listed there (except for the 0-index and the parent context)
is computable from the given list and the current index so I don't see a reason
to add them to the framework merely for convenience.

I suppose a reason is that this set of variables covers 99% of programmers' use cases with loops. So it is more than convenience, it is pragmatics. Otherwise, $index can be regarded just as a convenient feature as well. Of course, it is not, it is not only that kind of feature. But I agree that most of them are implemented much more easy if we have $index implemented than if we have not.

Every variable listed there (except for the 0-index and the parent context)
is computable from the given list and the current index so I don't see a reason
to add them to the framework merely for convenience.

I suppose a reason is that this set of variables covers 99% of programmers' use cases with loops. So it is more than convenience, it is pragmatics. Otherwise, $index can be regarded just as a convenient feature as well. Of course, it is not, it is not only that kind of feature. But I agree that most of them are implemented much more easy if we have $index implemented than if we have not.

@barkmadley

This comment has been minimized.

Show comment
Hide comment
@barkmadley

barkmadley Nov 23, 2011

Contributor

Actually I withdraw my assertion that we would have to pre-compute the other variables. Since they can be calculated from the one or two essential variables then we can provide them as dependent observables that defer evaluation.

Contributor

barkmadley commented Nov 23, 2011

Actually I withdraw my assertion that we would have to pre-compute the other variables. Since they can be calculated from the one or two essential variables then we can provide them as dependent observables that defer evaluation.

Mark Bradley added some commits Dec 22, 2011

Mark Bradley
Merge branch 'master' into foreach-context-index
Conflicts:
	spec/templatingBehaviors.js
	src/binding/editDetection/arrayToDomNodeChildren.js
	src/templating/templating.js
@jamesfoster

This comment has been minimized.

Show comment
Hide comment
@jamesfoster

jamesfoster Jan 27, 2012

Contributor

What is the status of this feature?

Contributor

jamesfoster commented Jan 27, 2012

What is the status of this feature?

@vamp

This comment has been minimized.

Show comment
Hide comment
@vamp

vamp Jan 30, 2012

errors of the current solution:

  1. $index not applied to child contexts
  2. $index not recalculated when one of child element was deleted

vamp commented Jan 30, 2012

errors of the current solution:

  1. $index not applied to child contexts
  2. $index not recalculated when one of child element was deleted
@SteveSanderson

This comment has been minimized.

Show comment
Hide comment
@SteveSanderson

SteveSanderson Jan 30, 2012

Contributor

What is the status of this feature?

Still keen to have this in the next point release (note that v1.3.1, which this was originally estimated for, is the same as v2.1.0 in the new numbering scheme).

errors of the current solution

Thanks for pointing that out - we will need to resolve that before this can become part of the master branch. I need to check the current suggested implementation carefully, as I think there will be some refactoring needed.

Contributor

SteveSanderson commented Jan 30, 2012

What is the status of this feature?

Still keen to have this in the next point release (note that v1.3.1, which this was originally estimated for, is the same as v2.1.0 in the new numbering scheme).

errors of the current solution

Thanks for pointing that out - we will need to resolve that before this can become part of the master branch. I need to check the current suggested implementation carefully, as I think there will be some refactoring needed.

@mbest

This comment has been minimized.

Show comment
Hide comment
@mbest

mbest Jan 31, 2012

Member

$index not applied to child contexts

One of the changes I made in #290 is to copy custom properties to the child context, which would fix this problem.

Member

mbest commented Jan 31, 2012

$index not applied to child contexts

One of the changes I made in #290 is to copy custom properties to the child context, which would fix this problem.

mbest added some commits Feb 23, 2012

Merge barkmadley:foreach-context-index into 182-foreach-index
Conflicts:
	spec/templatingBehaviors.js
	src/templating/templating.js
@mbest

This comment has been minimized.

Show comment
Hide comment
@mbest

mbest Feb 23, 2012

Member

Currently one of the new specs fails in IE because IE strips out some spaces. It's "Data binding 'foreach' option should update bindings that reference an $index if the list changes".

Member

mbest commented Feb 23, 2012

Currently one of the new specs fails in IE because IE strips out some spaces. It's "Data binding 'foreach' option should update bindings that reference an $index if the list changes".

@rniemeyer rniemeyer referenced this pull request Feb 24, 2012

Closed

2.1 release discussion #338

mbest added some commits Feb 25, 2012

foreach $index: fix spec that failed in IE; help performance a bit
renderTemplateForEach now assumes that setDomNodeChildrenFromArrayMapping will call the mapping callback and then the afterAdding callback once for each new or changed item. Modify specs to verify this behavior.
@vamp

This comment has been minimized.

Show comment
Hide comment
@vamp

vamp Mar 5, 2012

what about availability $array and $length variables context?

vamp commented Mar 5, 2012

what about availability $array and $length variables context?

@SteveSanderson

This comment has been minimized.

Show comment
Hide comment
@SteveSanderson

SteveSanderson Mar 5, 2012

Contributor

what about availability $array and $length variables context?

You can get this information already using $parent.someArray and $parent.someArray.length

Contributor

SteveSanderson commented Mar 5, 2012

what about availability $array and $length variables context?

You can get this information already using $parent.someArray and $parent.someArray.length

@vamp

This comment has been minimized.

Show comment
Hide comment
@vamp

vamp Mar 5, 2012

Steve, in this case I need to know variable name of parent context (it breaks creating of reusable bindings)...

vamp commented Mar 5, 2012

Steve, in this case I need to know variable name of parent context (it breaks creating of reusable bindings)...

@vamp

This comment has been minimized.

Show comment
Hide comment
@vamp

vamp Mar 20, 2012

what about:

        arrayItemContext['$array'] = arrayOrObservableArray;
        arrayItemContext['$length'] = length;

where (shared for all child contexts, outside executeTemplateForArrayItem)

    var length = ko.dependentObservable(function(){
        return (ko.utils.unwrapObservable(arrayOrObservableArray) || []).length;
    }, null, {'disposeWhenNodeIsRemoved': targetNode});

what about:

        arrayItemContext['$array'] = arrayOrObservableArray;
        arrayItemContext['$length'] = length;

where (shared for all child contexts, outside executeTemplateForArrayItem)

    var length = ko.dependentObservable(function(){
        return (ko.utils.unwrapObservable(arrayOrObservableArray) || []).length;
    }, null, {'disposeWhenNodeIsRemoved': targetNode});
@rniemeyer

This comment has been minimized.

Show comment
Hide comment
@rniemeyer

rniemeyer Mar 23, 2012

Member

Took another look at this one. Functionality works well and the simplified implementation is pretty straightforward. I do kind of wish that it was opt-in, so we can avoid creating an extra observable for each item when $index is not needed, although the performance overhead is likely minimal.

Member

rniemeyer commented Mar 23, 2012

Took another look at this one. Functionality works well and the simplified implementation is pretty straightforward. I do kind of wish that it was opt-in, so we can avoid creating an extra observable for each item when $index is not needed, although the performance overhead is likely minimal.

SteveSanderson added a commit that referenced this pull request Mar 23, 2012

Merge pull request #182 from SteveSanderson/182-foreach-index
added $index value in the binding context of foreach templates.

@SteveSanderson SteveSanderson merged commit f9db930 into master Mar 23, 2012

@SteveSanderson

This comment has been minimized.

Show comment
Hide comment
@SteveSanderson

SteveSanderson Mar 23, 2012

Contributor

Fantastic - this looks great. Thanks very much!

About perf, my quick foreach stress testing didn't show any significant extra cost to maintaining the $index observable. It was a lot slower if I actually referenced $index in my view (not surprising, because of course then a lot more of the view has to be redrawn whenever you mutate your array). But anyone who doesn't make use of $index shouldn't see any noticeable degradation of performance.

Contributor

SteveSanderson commented Mar 23, 2012

Fantastic - this looks great. Thanks very much!

About perf, my quick foreach stress testing didn't show any significant extra cost to maintaining the $index observable. It was a lot slower if I actually referenced $index in my view (not surprising, because of course then a lot more of the view has to be redrawn whenever you mutate your array). But anyone who doesn't make use of $index shouldn't see any noticeable degradation of performance.

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