Skip to content
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

Observable doesn't update view after ng2-redux update #259

Closed
AntJanus opened this Issue Nov 16, 2016 · 21 comments

Comments

Projects
None yet
5 participants
@AntJanus
Copy link

AntJanus commented Nov 16, 2016

I have a pretty simple app that I'm running and whenever the state updates, the view doesn't reflect that. Using Angular 2.2.0 and ng2-redux 4.1.0

Here's what I'm doing:

I have a select in my app.component.ts file

I'm using the async pipe in my template to display the raw data in app.html and every single time it comes back as empty.

I'm intercepting the actions and the reducers to console log wtf is going on. The actions run, the new state gets returned but the view doesn't update. I originally had this project running 4.0.0-beta.0 with angular 2.0.0 and that worked flawlessly. After update, doesn't work.

@SethDavenport

This comment has been minimized.

Copy link
Member

SethDavenport commented Nov 17, 2016

A few people have had issues when they updated Angular 2 but didn't update zonejs as well. What version of zonejs are you using?

@AntJanus

This comment has been minimized.

Copy link
Author

AntJanus commented Nov 17, 2016

I'm using 0.6.26. Maybe I'm initializing it wrong. In my main.ts (app entry), I'm doing the following:

require('core-js/shim')
require('zone.js')

(my local machine now uses imports).

I copied the vendor.ts file from the counter project but that didn't help anything.

@SethDavenport

This comment has been minimized.

Copy link
Member

SethDavenport commented Nov 17, 2016

let me see if I can repro ... I haven't gotten around to trying ng 2.2 yet

@e-schultz

This comment has been minimized.

Copy link
Member

e-schultz commented Nov 17, 2016

@AntJanus tried running your project to see if I could find out what's going on, but when running npm start I'm getting an error with electron.

eateCompilerHostFromConfigFileSync (/Users/evan/Github/e-schultz/omen/node_modules/electron-compile/lib/config-parser.js:361:19)
    at createCompilerHostFromProjectRootSync (/Users/evan/Github/e-schultz/omen/node_modules/electron-compile/lib/config-parser.js:381:12)
    at init (/Users/evan/Github/e-schultz/omen/node_modules/electron-compile/lib/config-parser.js:274:22)
    at main (/Users/evan/Github/e-schultz/omen/node_modules/electron-prebuilt-compile/lib/es6-init.js:38:29)
    at Object.<anonymous> (/Users/evan/Github/e-schultz/omen/node_modules/electron-prebuilt-compile/lib/es6-init.js:41:1)
    at Module._compile (module.js:556:32)
    at Object.Module._extensions..js (module.js:565:10)
    at Module.load (module.js:473:32)
    at tryModuleLoad (module.js:432:12)
    at Function.Module._load (module.js:424:3)

any ideas?

@scarrier

This comment has been minimized.

Copy link

scarrier commented Nov 17, 2016

I'm having the same issue with the view not updating. I tried updating the counter project to 2.2.0 and it still was working fine, so I don't think it's just angular.
I did get it working with the following style workaround. It just subscribes to the @select observable and updates a local object. Not an ideal solution, but it works.

@select('user') user: Observable<User>;
myUser: User;

ngOnInit() {
    this.user.subscribe(u => { this.myUser = u; } );
}
@AntJanus

This comment has been minimized.

Copy link
Author

AntJanus commented Nov 17, 2016

@e-schultz I'll look into that. I always use my globally installed electron package so I probably messed up there. It should run with npm run watch and electron . but obviously, I'll fix it.

@scarrier I'll try that out and let you know if that works!

@AntJanus

This comment has been minimized.

Copy link
Author

AntJanus commented Nov 17, 2016

@e-schultz fixed it. npm start will run the app (after npm install)

@AntJanus

This comment has been minimized.

Copy link
Author

AntJanus commented Nov 18, 2016

@scarrier I've no idea what I'm doing wrong but that workaround doesn't fix it. When I log out the contents of the subscribe function, I get the stream updates as they come in but the view just won't update.

@scarrier

This comment has been minimized.

Copy link

scarrier commented Nov 18, 2016

Did you update your template to use the local copy of the object? In my case the template looks like

<div> Welcome, {{myUser.username}} <div>

Just doing standard template binding, not using the async pipe on the Observable. If you see the updates coming in the subscribe, and you are updating your local object with the new value, then that should be reflected in your view by standard angular binding. If not, there might be something else going on.

@SethDavenport

This comment has been minimized.

Copy link
Member

SethDavenport commented Nov 18, 2016

@AntJanus are you using regular or on_push change detection?

@SethDavenport

This comment has been minimized.

Copy link
Member

SethDavenport commented Nov 18, 2016

What you describe is that the actions and reducers are firing and the state is updated, but Angular doesn't refresh its view right?

If so that sounds like a classic ZoneJS or change detection related issue.

If you're willing to share your repo I can take a look this weekend.

In the meantime some more questions:

  • are you dispatching actions via callback from a 3rd-party library?
    • can you import NgZone into your action creator and console log the value of NgZone. isInAngularZone()?
  • what (if any) redux middlewares are you using?
  • are you using AoT and/or the angular-cli?
@SethDavenport

This comment has been minimized.

Copy link
Member

SethDavenport commented Nov 18, 2016

Ok cool - just saw the link above, and that it's Electron. Will take a look soon.

@AntJanus

This comment has been minimized.

Copy link
Author

AntJanus commented Nov 18, 2016

I just wanted to quickly mention that I'm really appreciative for all the help everyone is providing! :)

@scarrier updated it. Using | async throws an error so I just did a direct binding. Still didn't work.

@SethDavenport thanks for taking a look! I didn't change the change detection strategy but yes: actions + reducers run, state is updated, even .subscribe gets run but the template doesn't update. It could definitely be a ZoneJS error but I'm not sure how to troubleshoot that. I'm still a n00b :)

As far as your questions, I'm not using any 3rd-party libraries to interact with NgRedux, there's no middleware, and I created the app from scratch.

Thanks for taking a look!

You can easily recreate the issue by trying to open one of the files from the sidebar (directories don't work yet!) once the electron app loads up. If you check the console, you'll see the output of the IPC connection (you'll get a response from the main thread with all the data). I'm not sure if I pushed this but on the main content area (so, not the sidebar) you should see an empty {} which is supposed to be a loaded file. Once you click on a file in the sidebar (like CHANGELOG.md), the empty curly braces should change and the entire page should display an editor but it doesn't.

@SethDavenport

This comment has been minimized.

Copy link
Member

SethDavenport commented Nov 19, 2016

By the way, very cool to see someone using ng2-redux with Electron. Haven't played with Electron much but it seems pretty awesome.

I cloned your repo and did some experimenting. You can see the results here: AntJanus/omen#4

As I suspected, it is related to dispatches happening outside the Angular zone. Basically what's happening is that a lot of your calls to dispatch are called from callbacks to Electron's ipcRenderer.

The way Angular2 works is that ZoneJS basically monkeypatches as many async APIs as it knows about in order to bring them into a setup where Angular knows that async stuff has 'settled'. At this point Angular2 fires its change detector and the UI updates.

In your case, the asynchrony is coming from an Electron API that Angular doesn't know about. This means that whatever dispatches are called from electron callbacks will fire, and run reducers, etc; but Angular2 doesn't know it has to update its UI.

My pr basically wraps ngRedux.dispatch in a function that checks whether it's in-zone or not; and if not it calls a special hook to do the dispatch in-zone instead.

@SethDavenport

This comment has been minimized.

Copy link
Member

SethDavenport commented Nov 19, 2016

This is actually a general with problem integrating callback-driven third-party APIs with Angular2 - think of it as analogous to the old ng1 digest cycle stuff: a cleaner, better implementation, but the same fundamental issue when faced with callbacks.

angular/angular#7154

@e-schultz this has come up a few times with different libraries - Electron, SignalR, etc. I'm wondering if we should take my wrapped dispatch function in AntJanus/omen#4 and make NgRedux.dispatch work that way by default? It's definitely part of 'bridging the gap' between 3rd-parties and Angular2, which is part of our mission.

@AntJanus

This comment has been minimized.

Copy link
Author

AntJanus commented Nov 19, 2016

@SethDavenport I had no idea this could be a problem! It sounds exactly like a $digest problem but I see the difference. Thanks for making the fix. I'll have to look more into how Zone works because I'll be migrating the IPC API to use the electron remote API but that wraps everything in Promises so I wonder if that'll change things.

The weird thing is that Angular 2.0 and an older version of ng2-redux worked flawlessly. I'm using that editor to write NaNoWriMo so I got good 20K words written in it and 20-30 files created/saved using this app.

Thanks for all the help again! Definitely makes me understand Angular 2 much more :)

@e-schultz

This comment has been minimized.

Copy link
Member

e-schultz commented Nov 21, 2016

@SethDavenport I've thought about doing a check to see if in the angular zone or not - and doing a zone.run.

I'm just thinking of times of which people might be explicitly not wanting to run things in a zone, basically if someone has intentionally done a ngZone. runOutsideAngular(()=>{}) because they want finer control over when CD kicks in - would this be a problem?

It seems like it'd be the less common scenario - but if someone is wanting that level of control over when to trigger CD/etc, wouldn't want to take that away.

The more common scenario though is, 'something I don't know is kicking me out of the zone for some reason' - and would be nice to have a better buffer against that.

@AntJanus

This comment has been minimized.

Copy link
Author

AntJanus commented Nov 21, 2016

@e-schultz I think it makes sense to not patch it. If Zone.js or Angular 2 add support for it, that's great; however, they might have a reason against it. It doesn't make sense to override it in this one very specific instance.

@SethDavenport

This comment has been minimized.

Copy link
Member

SethDavenport commented Nov 21, 2016

@e-schultz @AntJanus I guess what I'm saying is that it's not that specific: it comes up pretty frequently any time someone uses a third party lib with a callback.

I think people expect it to just work (TM), and there's a pretty simple solution. Also, we compare unfavourably to ngrx/store here (to be verified); my bet is that ngrx's dispatch automatically ends up in-zone because they implement dispatch as an Observable operator (I'm guessing Observable is one of the things zonejs knows to monkeypatch).

For the case of 'out of zone' updates @mhazy and I were arguing about that the other day. Personally I think it's "non-redux" in the sense that now your UI is no longer a pure function of your action stream over time.

However we don't want to restrict people if they really want to do it. I had suggested at the time making zone enforcement the default, but also providing an 'escape hatch':

dispatch(action: Action, runOutsideAngular = false)

or similar. Of course this messes with the Redux API in a way that's not friendly to middlewares. Maybe instead

dispatch(action: Action) { ... }

// Are you sure you need this?
dispatchOutsideAngular(action: Action) { ... }

SethDavenport pushed a commit to SethDavenport/ng2-redux that referenced this issue Jan 1, 2017

Seth Davenport
Ensure dispatch always happens In-Zone
This is to help resolve issues like angular-redux#259

SethDavenport pushed a commit to SethDavenport/ng2-redux that referenced this issue Jan 2, 2017

Seth Davenport
Ensure dispatch always happens In-Zone
This is to help resolve issues like angular-redux#259

SethDavenport pushed a commit to SethDavenport/ng2-redux that referenced this issue Jan 2, 2017

Seth Davenport
Ensure dispatch always happens In-Zone
This is to help resolve issues like angular-redux#259

SethDavenport pushed a commit to SethDavenport/ng2-redux that referenced this issue Jan 2, 2017

Seth Davenport
Ensure dispatch always happens In-Zone
This is to help resolve issues like angular-redux#259
@SethDavenport

This comment has been minimized.

Copy link
Member

SethDavenport commented Jan 2, 2017

Fix coming in 5.1

SethDavenport added a commit that referenced this issue Jan 2, 2017

Ensure dispatch always happens In-Zone (#276)
* Prelim work for 5.0.0-beta.0

* more comments in changelog

* Ensure dispatch always happens In-Zone

This is to help resolve issues like #259
@anton-107

This comment has been minimized.

Copy link

anton-107 commented Feb 2, 2017

hi guys, i'm having this issues (observables not emitting new events) with the following dependencies:

"dependencies": {
    "@angular/common": "^2.3.1",
    "@angular/compiler": "^2.3.1",
    "@angular/core": "^2.3.1",
    "@angular/forms": "^2.3.1",
    "@angular/http": "^2.3.1",
    "@angular/material": "^2.0.0-beta.1",
    "@angular/platform-browser": "^2.3.1",
    "@angular/platform-browser-dynamic": "^2.3.1",
    "@angular/router": "^3.3.1",
    "core-js": "^2.4.1",
    "ng2-redux": "^5.1.2",
    "redux": "^3.6.0",
    "redux-logger": "^2.8.1",
    "rxjs": "^5.0.1",
    "ts-helpers": "^1.1.1",
    "zone.js": "^0.7.2"
  }
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.