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

fix(pipe): isObservable usage #1411

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

iamnaumovnikita
Copy link

isObservable(res) instead of isObservable(res.subscribe)

I changed the usage of isObservable function to the correct one. Previously, it checks the subscribe function itself while it should check the observable itself. I also add a test that describes in which case it could lead to problem.

Without my fix the test is red and the actual result is

"{"operator": [Function anonymous], "source": {"_subscribe": [Function anonymous]}}"

image

With my fix the test is green.

image

In my test I described the scenario of a race condition when there is a bit delay of loading new language translation (after switching) and the usage MissingHandler with observable takes place.

P.S.

In many cases the next tick will solve the problem and user will see the correct translation, but there is a chance to see the stringified observable "{"operator": [Function anonymous], "source": {"_subscribe": [Function anonymous]}}"

In my PR I also removed some redundat imports.

iamnaumovnikita and others added 2 commits February 8, 2023 00:06
check observable itself instead of subscribe function
@iamnaumovnikita iamnaumovnikita changed the title This PR fixes the misusing of isObservable function in translate.pipe fix(pipe): isObservable usage Feb 9, 2023
CodeAndWeb added a commit to CodeAndWeb/ngx-translate that referenced this pull request Sep 25, 2024
Test for already fixed observable use.
@CodeAndWeb
Copy link
Member

Integrated in for ngx-translate v16
CodeAndWeb@e5558ff

@CodeAndWeb CodeAndWeb closed this Sep 25, 2024
@ocombe ocombe reopened this Sep 26, 2024
@ocombe
Copy link
Collaborator

ocombe commented Sep 26, 2024

In your fork v16, please do not make people confused

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

Successfully merging this pull request may close these issues.

3 participants