InfomediaLtd / angular2-redux-example

Example project for using pure Redux with Angular 2 and TypeScript
MIT License
71 stars 14 forks source link

Error on async call #2

Closed born2net closed 8 years ago

born2net commented 8 years ago

I tried to include the code in my project and getting an error of

dispatching (dispatch) {
                dispatch(_this.requestUsers());
                _this._http.get("" + BASE_URL)
                    .map(function (result) { return result.json(); })
                    .…
browser_adapter.js:85 EXCEPTION: Error during instantiation of AdminComponent!

ORIGINAL EXCEPTION: Error: Actions must be plain objects. Use custom middleware for async actions.

in AdminComponent

basically that async calls must go through the middleware... have you seen this before?

regards

basically

rubyboy commented 8 years ago

Interesting. I just tried getting a clean copy and it works well. Can you paste in the package.json and config.js files so we have exactly the same dependency versions?

rubyboy commented 8 years ago

Actually, even better, do you have the project public so I can take a look? Maybe you're not using thunks? Notice that all dispatches have to happen in a function that gets returned, which are "thunks" - they get the dispatch function as an argument and can dispatch other actions.

https://github.com/InfomediaLtd/angular2-redux-example/blob/master/app/main.ts#L30

born2net commented 8 years ago

sure and thank you for checking, it's the Redux branch: https://github.com/born2net/ng2Boilerplate/tree/Redux

born2net commented 8 years ago

and this is specifically for your code: https://github.com/born2net/ng2Boilerplate/tree/Redux/src/comps/app3

born2net commented 8 years ago

I think I may found the issue tx to your comment, I believe my thunkMiddleware is missing

rubyboy commented 8 years ago

No worries. Let me know if you're having trouble adding that in. Basically, you want to add it as a middleware. https://github.com/gaearon/redux-thunk

born2net commented 8 years ago

tx, I remembred why I took it out, becuase I has another issue with it, some off BrowserDomAdapter.logError from ng2... so I am checking again now, let me ask you if I may, what is the indow.devToolsExtension for?

born2net commented 8 years ago

ya same issue with that Thunk, error TS2304: Cannot find name 'BrowserDomAdapter'

rubyboy commented 8 years ago

That's something really awesome that for me is one of the most significant benefits of redux - it gives you visibility of all actions happening. It's the redux devtools extension for Chrome: https://github.com/zalmoxisus/redux-devtools-extension.

rubyboy commented 8 years ago

Looking at it now. How do I reproduce it? Should I run the app from the main app.html or something deeper?

rubyboy commented 8 years ago

Nope. It's ng2 as well :) I'm not using React at all (no experience with it whatsoever)

born2net commented 8 years ago

just saw it's for Redux... wow... installing :)

born2net commented 8 years ago

here is the error I get in the console: capture

rubyboy commented 8 years ago

Looks like a problem with the import, maybe? I'm still waiting for "npm install" to finish... Quite a project :)

rubyboy commented 8 years ago

NPM finished with an error (see below). Trying to run "gulp LiveServer" starts a browser on 8080 but it get a 404. 3001 shows that BrowserSync is running, but I'm not sure why 8080 doesn't serve the app. Ideas?

Running in build mode: true 10% 0/2 build modulests-loader: Using typescript@1.7.5 and /Users/rubyboy/Development/Workspace/ 3rd/ng2Boilerplate/tsconfig.json 7737ms build modules 26ms seal 54ms optimize 41ms hashing 81ms create chunk assets

22ms additional chunk assets 2111ms optimize chunk assets 1518ms optimize assets Hash: 1ffbd575df64cc88f9ef Version: webpack 1.12.12 Time: 11619ms

ERROR in Cannot find module 'less' @ ./~/style-loader!./~/css-loader!./~/less-loader!./~/bootstrap-webpack/bootstrap-styles.loader. js!./~/bootstrap-webpack/bootstrap.config.js 4:14-128

ERROR in Cannot find module 'less' @ ./~/style-loader!./~/css-loader!./~/less-loader!./~/font-awesome-webpack/font-awesome-styles.l oader.js!./~/font-awesome-webpack/font-awesome.config.js 4:14-134

born2net commented 8 years ago

ya I need to remove live-server as I no longer use it, I use the webpack server now, so you can ignore that and just run webpack-dev-server --nolazy --debug_mem --inline --progress --colors --display-error-details --display-cached --port=8080

rubyboy commented 8 years ago

Looks a bit better, showing the progress thingy, but it just sits there. I only see two network requests - localhost (with a small HTML coming back) and loading.svg.

rubyboy commented 8 years ago

BTW, are you using the TypeScript compiler as a "ts" loader or Babel?

born2net commented 8 years ago

ts

born2net commented 8 years ago

I have a feeling something is wrong with my const createStoreWithMiddleware = compose(applyMiddleware(thunkMiddleware, Lib.loggerMiddleware))(createStore);

rubyboy commented 8 years ago

Any idea why it's not running?

Can you try the following? import * as thunkMiddleware from "redux-thunk"

rubyboy commented 8 years ago

Well, the first thing I would have done (if I managed to run it locally :) ) is do a "console.log("thunkMiddleware",thunkMiddleware);" right after the Babel comment in starwars.ts. See if you're getting the function imported properly. I have a feeling it's not and the problem is the import statement.

born2net commented 8 years ago

checking...

rubyboy commented 8 years ago

If it does import something it may still be wrong, because it bring in something that has a default. Try printing it out and checking what it is.

For me, for example, when I use the * syntax above I need to use thunkMiddleware.default to get to the actual function

born2net commented 8 years ago

ok great, gotta go to a 45 min meeting so will try right after, tx!!!!

rubyboy commented 8 years ago

No worries. Let me know if that doesn't work and I'll check that tomorrow. Would be good to get to a point I can run it locally so I can easily figure it out. Pretty sure it's the import and some collision with Babel/Webpack/something.

born2net commented 8 years ago

u d man, fixed it like u said with the import:

import * as thunk from 'redux-thunk';

const createStoreWithMiddleware = compose(applyMiddleware( thunk, Lib.loggerMiddleware), typeof window === 'object' && typeof window.devToolsExtension !== 'undefined' ? window.devToolsExtension() : f => f)(createStore);

and I LOOOOOOOOOOOVE the devTools ;)

born2net commented 8 years ago

everything works great... quick q, will this leak memory as we call subscribe on every fetch?

fetchFilms() {
        return (dispatch) => {
            dispatch(this.requestFilms());

            this._http.get(`${BASE_URL}`)
                .map(result => result.json())
                .map(json => {
                    dispatch(this.receiveFilms(json.results));
                    dispatch(this.receiveNumberOfFilms(json.count));
                })
                .subscribe();
        };
    }
rubyboy commented 8 years ago

Glad to hear it all works well :)

I don't think it should leak, because the object is only used within that function so once it ends (when the results come back from the server) it would be cleaned up. My understanding is that Rx subscriptions need to be cleaned up only if they are kept inside components or other containers.

Related question for you: Do you think we need to unsubscribe from the appStore in the components by doing that in the onDestroy lifecycle method? I wasn't sure if it's needed or not, given that we don't really keep a reference to the store and it should be destroyed as the component is cleaned up. Also, as much as I checked I couldn't find any leaks or misbehaviour so I didn't do that. Lastly, I really don't like the idea of keeping a reference and adding the onDestory boilerplate method, so I would love to not have to unsubscribe. Let me know if you find out this is required and I'll add it to the code.

born2net commented 8 years ago

I reviewed the code and I think all is good... no need to unsub you are right... The two things I would love to see (and will prob do in a bit) is add the Immutable.js and some Rxjs sugar https://github.com/acdlite/redux-rx

born2net commented 8 years ago

One question I have is on your Films-Component for example, is it "aware" of redux as it uses dispatch etc... so doesn't that break "encapsulation" as the entire film component and it's children should not know anything about Redux? i.e.: should just be given the Redux data...?

tx Sean

rubyboy commented 8 years ago

Redux-rx looks sweet! First time I see this extension for redux.

In regards to your question: There's a clear dichotomy between the smart components and the dumb views. The views know nothing about state, redux or composition. They are just simple visual components that expose properties and events. Then smart container components are the ones that do know about the app store, subscribing to its state changes and dispatching actions (coming from action creators). I guess I could have exposed all the smartness to be done in the app container, but that wouldn't hold for any app that grows. So, it makes sense to create a set of smart components that know about redux. Ideally, the more dumb views you have the better, but at some point you have to have something to compose them in UI and tie them to the store, and that's what the smart container components do.

Does that make sense? Do you have other ideas about composition of components in this example?

BTW, I'm contemplating about separating the Http code from the action creators to services. It's not a lot of code, so I'm not sure it's worth it, but it may clean up the action creators from knowing about Http and put all the service logic in a central place... Any thoughts about that?

Cheers, Ruby

rubyboy commented 8 years ago

Is redux-rx for React only? It's got react in its dependencies...

born2net commented 8 years ago

I don't think so because it was prompted from the Redux docs... and tx for the update above... and no, no other better ideas, I will follow the same guidelines in my new App ;)

I did fix the npm install if you have any interest in the app I developed: demo: http://ng2.javascriptninja.io

born2net commented 8 years ago

Another note, as for example on

fetchFilm(index) {
        return (dispatch) => {        
            dispatch(this.requestFilm());
            this._http.get(`${BASE_URL}${index + 1}/`)
                .map(result => result.json())
                .map(json => {
                    dispatch(this.receiveFilm(json));
                })
                .subscribe();
        };
    }

we do actually call Subscribe on every single call. which could leak on a large app (lots of new daily movies for example...)

maybe this would be better:

fetchFilm(index) {
        return (dispatch) => {
            dispatch(this.requestFilm());
            var sub = this._http.get(`${BASE_URL}${index + 1}/`)
                .map(result => result.json())
                .map(json => {
                    dispatch(this.receiveFilm(json));
                })
                .subscribe(e=>sub.unsubscribe());
        };
    }
rubyboy commented 8 years ago

I don't mind adding the unsubscribe, but I still wanted to check if there's a leak so here's what I did:

I've updated the film action to create a 30MB variable for every time it gets called:

fetchFilms() {
        return (dispatch) => {
            dispatch(this.requestFilms());

            let a = [];
            this._http.get(`${BASE_URL}`)
                .map(result => result.json())
                .map(json => {
                    const t = JSON.stringify(json.results);
                    for (var j=0; j<100; j++) {
                      let big = "";
                      for (var i=0; i<10000; i++) {
                        big = big + t;
                      }
                      a.push(big);
                      //console.log(j + "" + big.length);
                    }
                    dispatch(this.receiveFilms(json.results));
                    dispatch(this.receiveNumberOfFilms(json.count));
                })
                .subscribe();
        };
    }

Then, updated the films component to retrieve films on an interval:

constructor(private _appStore:AppStore,
                private _filmActions:FilmActions) {

        _appStore.subscribe((state) => {
            this.filmsCount = state.films.count;
            this.currentFilm = state.films.currentFilm;
            this.isFetchingCurrentFilm = state.films.isFetchingFilm;
        });

        setInterval(() => {
          this.counter++;
          this._appStore.dispatch(this._filmActions.fetchFilms());
        },500);
    }

Looking at the Chrome memory tool I can see it allocating 30MB on every call, but then it drops, as expected: image

So, I'm still not convinced there is a leak here. Any ideas on other tests we can perform to confirm it? Again, not that I mind adding the "unsubscribe". It's probably cleaner anyway, but I would really like to understand what is the right way to do it and why do I need to worry about a leak.

rubyboy commented 8 years ago

I had another idea and looked at the Http service source. Here's the important line from Angular's Http XHR backend source: image

Notice how it runs the "complete" after getting the result. This means it actually unsubscribes on completion. So you don't need to do it yourself.

To confirm that, here's my new code for the fetchFilms method:

fetchFilms() {
        return (dispatch) => {
            dispatch(this.requestFilms());

            let observer = this._http.get(`${BASE_URL}`)
                .map(result => result.json())
                .map(json => {
                    dispatch(this.receiveFilms(json.results));
                    dispatch(this.receiveNumberOfFilms(json.count));
                    console.log("2 isUnsubscribed",observer.isUnsubscribed);
                    window.setTimeout(() => {
                      console.log("3 isUnsubscribed",observer.isUnsubscribed);
                    },10);
                })
                .subscribe();
            console.log("1 isUnsubscribed",observer.isUnsubscribed);
        };
    }

As expected, you can see that it is always unsubscribed automatically after getting the result and finishing with the observable operators. This happens on a timeout (#3) so we can check the status of the observable when it's all done and completed. image

I think this concludes that there is no need to unsubscribe from the Http service :) Agree?

Cheers, Ruby

born2net commented 8 years ago

I see, PERFECT, tx for the effort...