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

Fix `can.List` event docs, as well as an event ambiguity #998

Closed
zkat opened this Issue May 18, 2014 · 2 comments

Comments

Projects
None yet
3 participants
@zkat
Contributor

zkat commented May 18, 2014

Summary

The can.List docs for event binding seem to be wrong about the removedElements value we actually get, and the add event actually returns a single value sometimes, which creates an ambiguity when listening to that event. Finally, .pop()ing an empty can.List gets you [undefined] as the removedElements value, which also creates an ambiguity.

Details

Documentation corrections

The docs for can.List say:

(for the add event)

* newElements - The new elements. If more than one element is added, newElements will be an array. Otherwise, it is simply the new element itself.

(for the remove event)

* removedElements - The removed elements. If more than one element was removed, removedElements will be an array. Otherwise, it is simply the element itself.

I could not find a case for the latter, since can.List#removeAttr() gives an empty array as removedElements, which is a pretty reasonable value. If that really is the case, the remove event docs should be updated to reflect this, and possibly even point out that you get an empty array back.

Add event is ambiguous for out-of-bounds .attr() calls

For the add event, I couldn't find any regular can.List methods that triggered a newElements that wasn't an array of the actual values, except for can.List#attr, which does return the single item added. (EDIT: I forgot to mention that this is only for out-of-bounds indices) This creates a pretty problematic ambiguity, since users will have no way of distinguishing an add event where multiple elements were added from an event where a single array was added at a single index. Unless I'm missing something, this should probably be fixed, and the docs updated accordingly to note how newElements will always be an array.

Remove event is ambiguous for .pop() on empty lists

(EDIT: I found this one not long after the above)
For the remove event, calling can.List#pop() on an empty list gives us the following information for the change and remove events:

  • change - index: "0", newVal: undefined, oldVal: [undefined]
  • remove - index: 0, newVal: [undefined], oldVal: N/A (no such thing)

But if I have new can.List([undefined]), and call .pop() on that list, I get the exact same result.

This ambiguity is not present if you call .splice(0, 1) on both the [undefined] and empty lists.

Change events return index as string instead of integer

The add, set, and remove events all return their index as an integer, but the change event still uses a string. While we do want to (I assume) support .attr() on non-numberical properties of lists (since they kinda also work like objects/maps), I think we should emulate the behavior of Arrays and treat integer strings as integers, as well as in the interest of not surprising some poor soul who decides to switch events and suddenly gets "12" instead of 3.

@zkat zkat added the Bug label May 18, 2014

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer May 18, 2014

Contributor

Yes, I think it will always be an array

Sent from my iPhone

On May 18, 2014, at 3:19 AM, Josh Marchán notifications@github.com wrote:

Summary

The can.List docs for event binding seem to be wrong about the removedElements value we actually get, and the add event actually returns a single value sometimes, which creates an ambiguity when listening to that event.

Details

The docs for can.List say:

(for the add event)

  • newElements - The new elements. If more than one element is added, newElements will be an array. Otherwise, it is simply the new element itself.
    (for the remove event)
  • removedElements - The removed elements. If more than one element was removed, removedElements will be an array. Otherwise, it is simply the element itself.
    I could not find a case for the latter, since can.List#removeAttr() gives an empty array as removedElements, which is a pretty reasonable value. If that really is the case, the remove event docs should be updated to reflect this, and possibly even point out that you get an empty array back.

For the add event, I couldn't find any regular can.List methods that triggered a newElements that wasn't an array of the actual values, except for can.List#attr, which does return the single item added. This creates a pretty problematic ambiguity, since users will have no way of distinguishing an add event where multiple elements were added from an event where a single array was added at a single index. Unless I'm missing something, this should probably be fixed, and the docs updated accordingly to note how newElements will always be an array.


Reply to this email directly or view it on GitHub.

Contributor

justinbmeyer commented May 18, 2014

Yes, I think it will always be an array

Sent from my iPhone

On May 18, 2014, at 3:19 AM, Josh Marchán notifications@github.com wrote:

Summary

The can.List docs for event binding seem to be wrong about the removedElements value we actually get, and the add event actually returns a single value sometimes, which creates an ambiguity when listening to that event.

Details

The docs for can.List say:

(for the add event)

  • newElements - The new elements. If more than one element is added, newElements will be an array. Otherwise, it is simply the new element itself.
    (for the remove event)
  • removedElements - The removed elements. If more than one element was removed, removedElements will be an array. Otherwise, it is simply the element itself.
    I could not find a case for the latter, since can.List#removeAttr() gives an empty array as removedElements, which is a pretty reasonable value. If that really is the case, the remove event docs should be updated to reflect this, and possibly even point out that you get an empty array back.

For the add event, I couldn't find any regular can.List methods that triggered a newElements that wasn't an array of the actual values, except for can.List#attr, which does return the single item added. This creates a pretty problematic ambiguity, since users will have no way of distinguishing an add event where multiple elements were added from an event where a single array was added at a single index. Unless I'm missing something, this should probably be fixed, and the docs updated accordingly to note how newElements will always be an array.


Reply to this email directly or view it on GitHub.

@zkat zkat self-assigned this Jun 19, 2014

@zkat

This comment has been minimized.

Show comment
Hide comment
@zkat

zkat Sep 16, 2014

Contributor

Bump. These changes would be pretty nice to have.

Contributor

zkat commented Sep 16, 2014

Bump. These changes would be pretty nice to have.

@daffl daffl added this to the 2.2.0 milestone Feb 9, 2015

@daffl daffl closed this Feb 13, 2015

@daffl daffl removed the fixed in branch label Feb 20, 2015

@zkat zkat removed their assignment Oct 29, 2015

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