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

translate.get(...).flatMap(...) not working in pending state #308

Closed
jonaszuberbuehler opened this issue Nov 4, 2016 · 7 comments · Fixed by #329
Closed

translate.get(...).flatMap(...) not working in pending state #308

jonaszuberbuehler opened this issue Nov 4, 2016 · 7 comments · Fixed by #329

Comments

@jonaszuberbuehler
Copy link

I'm submitting a ... (check one with "x")

[x] bug report => check the FAQ and search github for a similar issue or PR before submitting
[x] support request => check the FAQ and search github for a similar issue before submitting
[ ] feature request

Not sure if it's a bug or misuse by me...

Current behavior
Using translate.get('...').flatMap(...).subscribe(observer) does not work if this.pending is set.

Expected/desired behavior
The observer gets called with the translation.

Reproduction of the problem
Made a small plunker. Calling doIt() from ngOnInit() does not work, calling it afterwards works just fine. Although I didn't dig too deep it looks like to be caused by this.pending.

@SamVerschueren
Copy link
Collaborator

I can confirm this seems like a bug. Have to dive deeper to find the cause.

@imgx64
Copy link
Contributor

imgx64 commented Nov 6, 2016

I submitted a PR to fix this issue. I used mergeMap instead of Observable.create and it worked. I'm not sure why, but the Observable.create closure was being called with a stopped observer.

@imgx64
Copy link
Contributor

imgx64 commented Nov 6, 2016

Ok, I found the root issue. getTranslation was using share() which was creating a hot observable, and then immediately subscribing to it. Once this subscription was done, the pending observable stopped because there are no more subscribers. Any subscribers afterwards were ignored.

@ocombe
Copy link
Collaborator

ocombe commented Nov 10, 2016

There's a good reason for adding the share, otherwise it will request multiple times the json file from the server (or anything else that a custom loader can do) if you have multiple translations on your page. Imagine a page with 10 translated words, then it would make 10 http requests.
I'll take a look at the problem but removing share is not an option.

@imgx64
Copy link
Contributor

imgx64 commented Nov 10, 2016

TranslateService.getTranslation gets called only once the first time a language is used (or if you explicitly call it or call reloadLang). It doesn't get called for every translated word.

But even if getTranslation is called multiple times, it would still do multiple HTTP requests even with the share, because it's creating a new observable every time it's called, not reusing the shared observable.

I don't think share is what you want. share uses refCount, so once all subscriptions are done, the shared observable unsubscribes from the original (http) observable and stops emitting. Any observers that subscribe afterwards simply never get called (neither onNext nor onComplete).

@imgx64
Copy link
Contributor

imgx64 commented Nov 20, 2016

Okay. I found the root root root issue!

It's a bug in RxJS (ReactiveX/rxjs#2145) that affects hot observables. I've added a PR that fixes this bug without removing the share().

ocombe pushed a commit that referenced this issue Nov 20, 2016
There's a bug in RxJS that breaks hot observables if one subscriber ignores errors (see ReactiveX/rxjs#2145 ), which is causing #308. This patch fixes it by always handling observable errors (handling errors is a good idea anyway).

Fixes #308
@ocombe
Copy link
Collaborator

ocombe commented Nov 20, 2016

Thanks a lot for taking the time to find another solution and submitting a PR, I've merged it!

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

Successfully merging a pull request may close this issue.

4 participants