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

new property isObservableArray, check value is array when using observable array #906

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
@sharkboy1976
  1. Added new property isObservableArray for observable arrays, so it is easier to determine if property is of observable array or just observable.

  2. When an observable array gets initialized other then array, null or undefined an error is thrown. When passing a value other than array, null or undefined to an already initialized observable array, no error is thrown and the observable array loses array functionality (can't push or pull anymore) and isn't observable anymore.

-New property isObservableArray for observable array
-Check for array values when updating observable arrays
@rniemeyer

This comment has been minimized.

Show comment
Hide comment
@rniemeyer

rniemeyer Apr 2, 2013

Member

We should consider this with #706 and #619. This implementation is different.

Member

rniemeyer commented Apr 2, 2013

We should consider this with #706 and #619. This implementation is different.

@sharkboy1976

This comment has been minimized.

Show comment
Hide comment
@sharkboy1976

sharkboy1976 Apr 2, 2013

looks like #706 isn't implemented in production jet. besides having only a bool property is easier and faster than having a function.

looks like #706 isn't implemented in production jet. besides having only a bool property is easier and faster than having a function.

@rniemeyer

This comment has been minimized.

Show comment
Hide comment
@rniemeyer

rniemeyer Apr 2, 2013

Member

Just was linking the issues together to consider them at the same time.

Member

rniemeyer commented Apr 2, 2013

Just was linking the issues together to consider them at the same time.

@sharkboy1976 sharkboy1976 reopened this Apr 2, 2013

@sharkboy1976

This comment has been minimized.

Show comment
Hide comment
@sharkboy1976

sharkboy1976 Apr 2, 2013

wrong button

wrong button

@sharkboy1976

This comment has been minimized.

Show comment
Hide comment
@sharkboy1976

sharkboy1976 Apr 2, 2013

actually i put two requests with the change request. my title doesn't state this clearly.

actually i put two requests with the change request. my title doesn't state this clearly.

@paglias

This comment has been minimized.

Show comment
Hide comment
@paglias

paglias Apr 5, 2013

Contributor

I see two problems with this:

  • it's a boolean which is faster of a function (but it's not this that will slow down your page) but a boolean can also be modified.
  • It's attached to the object and not external to it like ko.isObservable and trying it on a non observable array property will return undefined which I don't think is the best thing to have
Contributor

paglias commented Apr 5, 2013

I see two problems with this:

  • it's a boolean which is faster of a function (but it's not this that will slow down your page) but a boolean can also be modified.
  • It's attached to the object and not external to it like ko.isObservable and trying it on a non observable array property will return undefined which I don't think is the best thing to have
@sharkboy1976

This comment has been minimized.

Show comment
Hide comment
@sharkboy1976

sharkboy1976 Aug 15, 2013

it seems no one is taking care of the second bug

it seems no one is taking care of the second bug

@jrsearles

This comment has been minimized.

Show comment
Hide comment
@jrsearles

jrsearles Mar 15, 2014

I think the implementation in #706 is more consistent with the rest of the library (ko.isObservable, ko.isWriteableObservable, etc)

I think the implementation in #706 is more consistent with the rest of the library (ko.isObservable, ko.isWriteableObservable, etc)

@mbest mbest added close and removed close labels Mar 20, 2014

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