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

Feature Request - Nested Bindable Observables #2457

Closed
sitefinitysteve opened this Issue Jul 14, 2016 · 10 comments

Comments

Projects
None yet
7 participants
@sitefinitysteve
Contributor

sitefinitysteve commented Jul 14, 2016

Reference Issue: #2436 (comment)

Basically right now we need to do this:

viewModel = new Observable({
  filter: {
    labels: new Observable({
      apply: "",
      cancel: "",
    })
  }
});

In order for child properties to notify of changes, it's not intuitive and a tad overly complex.

KendoUI on the otherhand manages to have a single root observable and every child property at any level just becomes observable with no extra work... reducing code complexity, potential slack\SO questions... and it just looks nicer

viewModel = new Observable({
    filter: {
            labels: {
                apply: "",
                cancel: "",
            }
    }
});

Kendo Sample: http://dojo.telerik.com/OCUkA

@nsndeck

This comment has been minimized.

Show comment
Hide comment
@nsndeck

nsndeck Jul 19, 2016

Contributor

Done with commit #2469

Contributor

nsndeck commented Jul 19, 2016

Done with commit #2469

@nsndeck nsndeck closed this Jul 19, 2016

@nsndeck nsndeck added this to the 2.2.0 milestone Jul 19, 2016

@sitefinitysteve

This comment has been minimized.

Show comment
Hide comment
@sitefinitysteve

sitefinitysteve Jul 19, 2016

Contributor

HOORAY @nsndeck !

Contributor

sitefinitysteve commented Jul 19, 2016

HOORAY @nsndeck !

@hamorphis hamorphis added the done label Jul 20, 2016

@mnahkies

This comment has been minimized.

Show comment
Hide comment
@mnahkies

mnahkies Jul 29, 2016

@nsndeck I'd just like to point out that whilst I agree that the previous behaviour was somewhat unintuitive, that the new behaviour should be clearly noted and possibly even considered a breaking change.

I'm actually getting a runtime exception with this change:

file:///app/tns_modules/data/observable/observable.js:141:34: JS ERROR TypeError: undefined is not an object (evaluating 'this.disableNotifications[data.propertyName] = true')

Can't see any obvious cause of this, but I do currently have complex object hierarchies attached to observables that I didn't expect or intend to become recursively observable.

mnahkies commented Jul 29, 2016

@nsndeck I'd just like to point out that whilst I agree that the previous behaviour was somewhat unintuitive, that the new behaviour should be clearly noted and possibly even considered a breaking change.

I'm actually getting a runtime exception with this change:

file:///app/tns_modules/data/observable/observable.js:141:34: JS ERROR TypeError: undefined is not an object (evaluating 'this.disableNotifications[data.propertyName] = true')

Can't see any obvious cause of this, but I do currently have complex object hierarchies attached to observables that I didn't expect or intend to become recursively observable.

@spstratis

This comment has been minimized.

Show comment
Hide comment
@spstratis

spstratis Aug 11, 2016

@mnahkies I'm having the same issue as you now that I updated to 2.2.0. What was your approach to get around it if you don't mind me asking. I find that in certain cases just removing any nested Observables seems to fix the issue.

spstratis commented Aug 11, 2016

@mnahkies I'm having the same issue as you now that I updated to 2.2.0. What was your approach to get around it if you don't mind me asking. I find that in certain cases just removing any nested Observables seems to fix the issue.

@tjvantoll

This comment has been minimized.

Show comment
Hide comment
@tjvantoll

tjvantoll Aug 11, 2016

Contributor

While I’m a big fan of this feature, this change is a fairly major breaking change that will affect a whole lot of people.

The big problem I hit is that I was using an ObservableArray within an Observable, and when I upgraded to 2.2 I get the runtime exception that @mnahkies mentions above. I can get the error to go away by switching my ObservableArray to a vanilla Array, but, I then have problems getting ListViews to subscribe to the plain array appropriately.

I’m going to see what we can do to address this asap. At a minimum we need documentation explaining the changes.

Contributor

tjvantoll commented Aug 11, 2016

While I’m a big fan of this feature, this change is a fairly major breaking change that will affect a whole lot of people.

The big problem I hit is that I was using an ObservableArray within an Observable, and when I upgraded to 2.2 I get the runtime exception that @mnahkies mentions above. I can get the error to go away by switching my ObservableArray to a vanilla Array, but, I then have problems getting ListViews to subscribe to the plain array appropriately.

I’m going to see what we can do to address this asap. At a minimum we need documentation explaining the changes.

@spstratis

This comment has been minimized.

Show comment
Hide comment
@spstratis

spstratis Aug 11, 2016

@tjvantoll Thanks for the feedback. Is it possible for devs to rollback their TNS versions? If so could you link to a doc that outlines the process? Thanks!

spstratis commented Aug 11, 2016

@tjvantoll Thanks for the feedback. Is it possible for devs to rollback their TNS versions? If so could you link to a doc that outlines the process? Thanks!

@tjvantoll

This comment has been minimized.

Show comment
Hide comment
@tjvantoll

tjvantoll Aug 11, 2016

Contributor

@spstratis Yep.

  1. Remove your node_modules and platforms folders.
  2. Change the "tns-android", "tns-ios", and "tns-core-modules" version numbers in your package.json back to 2.1.0.
  3. Run tns install to use the old values.

There’s a chance you’ll also have to revert your NativeScript CLI version, but you’ll probably be fine running the 2.2 CLI with older versions of the modules and runtimes.

Contributor

tjvantoll commented Aug 11, 2016

@spstratis Yep.

  1. Remove your node_modules and platforms folders.
  2. Change the "tns-android", "tns-ios", and "tns-core-modules" version numbers in your package.json back to 2.1.0.
  3. Run tns install to use the old values.

There’s a chance you’ll also have to revert your NativeScript CLI version, but you’ll probably be fine running the 2.2 CLI with older versions of the modules and runtimes.

@mnahkies

This comment has been minimized.

Show comment
Hide comment
@mnahkies

mnahkies Aug 11, 2016

@spstratis we've been working around this by not upgrading our release branches, and by reverting the change to tns-core-modules/data/observable/observable.ts by pull request #2469 otherwise

mnahkies commented Aug 11, 2016

@spstratis we've been working around this by not upgrading our release branches, and by reverting the change to tns-core-modules/data/observable/observable.ts by pull request #2469 otherwise

@nsndeck

This comment has been minimized.

Show comment
Hide comment
@nsndeck

nsndeck Aug 12, 2016

Contributor

Hi all,
A little update from me. I've just push a change in the master branch (will be available with @next build, also we think for an option to make quick 2.2.1 release to be available officially). The change is some kind of a breaking. We marked Observable constructor that takes json object as deprecated (it is still working as before - creates an Observable with properties and sets values (one level only)), but later we should delete it. We replaced it with two static functions fromJSON and fromJSONRecursive that will do its job while giving an option to create Observables for all nested objects or not.

Issue mentioned by @mnahkies and @tjvantoll (thanks a lot for the hint and info about error) is fixed. The problem comes from the fact that ObservableArray is actually not a javascript array (Array.isArray(observableArray) returns false) and the code is trying to make it Observable.

Contributor

nsndeck commented Aug 12, 2016

Hi all,
A little update from me. I've just push a change in the master branch (will be available with @next build, also we think for an option to make quick 2.2.1 release to be available officially). The change is some kind of a breaking. We marked Observable constructor that takes json object as deprecated (it is still working as before - creates an Observable with properties and sets values (one level only)), but later we should delete it. We replaced it with two static functions fromJSON and fromJSONRecursive that will do its job while giving an option to create Observables for all nested objects or not.

Issue mentioned by @mnahkies and @tjvantoll (thanks a lot for the hint and info about error) is fixed. The problem comes from the fact that ObservableArray is actually not a javascript array (Array.isArray(observableArray) returns false) and the code is trying to make it Observable.

@tjvantoll

This comment has been minimized.

Show comment
Hide comment
@tjvantoll

tjvantoll Aug 19, 2016

Contributor

I confirmed that using switching my example from new Observable({}) to Observable.fromJSON({}) fixed the issue for me using “next”. Unfortunately though it doesn’t look like this fix made it into 2.2.1, so we’ll have to stick with “next” to workaround the problem until 2.2.2.

Contributor

tjvantoll commented Aug 19, 2016

I confirmed that using switching my example from new Observable({}) to Observable.fromJSON({}) fixed the issue for me using “next”. Unfortunately though it doesn’t look like this fix made it into 2.2.1, so we’ll have to stick with “next” to workaround the problem until 2.2.2.

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