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

valueChanges Observable from one param is leaking Events from other params of the same group. #158

Open
osahner opened this issue Jun 15, 2020 · 6 comments
Labels
Comp: Core Core functionality of ngqp Priority: Medium Users can work without this for now, but this should be fixed soon-ish Status: Needs Design There is no clear API for this feature yet and one should be designed before work on ican be started Type: Bug Something that should work doesn't

Comments

@osahner
Copy link

osahner commented Jun 15, 2020

Which versions are you using?

Angular: 9.1.11
@ngqp: 1.0.2

What does the bug look like?

The valueChanges Observable from one param is leaking Events from other params of the same group.

How can this bug be reproduced?

https://stackblitz.com/edit/angular-ivy-exykie?file=src%2Fapp%2Fhello.component.ts

Additional context

@osahner osahner added Status: Needs Triage We need to properly tag this issue Type: Bug Something that should work doesn't labels Jun 15, 2020
@Airblader
Copy link
Collaborator

Thanks for reporting an issue and providing a Stackblitz! Just to be clear, the issue you are bringing up is that when I change the second dropdown, an event emits for select1 (but still with the correct value), correct?

@osahner
Copy link
Author

osahner commented Jun 15, 2020

Yes. Sorry for the short description - still playing around with some other issues I have.
distinctUntilChanged would be the workaround.

@Airblader
Copy link
Collaborator

Thanks for clarifying! This does deserve a bit of background explanation. A fundamental design decision in ngqp is to make the route the single source of truth. This means that changing the dropdown writes to the URL, and ngqp listens to the URL to synchronize the model from. These are essentially two independent processes.

So in this case, changing the value updates the URL, and then because ngqp also listens for the URL, we get a navigation event and from there emit on the observables. The problem, however, is that we don't know which parameters changed with this navigation event.

Of course we could bake distinctUntilChanged into ngqp directly. But this would mean no-op value changes would not emit, which is also unexpected, and not in line with Angular forms. An even bigger reason is that with the current way the user has both options (by adding distinctUntilChanged or not), but if we bake it into ngqp we take away this option from the user.

In principle I think this is all still true, adding distinctUntilChanged is easy enough, and as such I'm incline to say this works as intended. A quick thought I just had, however, is that we could possibly pass information on which parameters have been changed in the navigation state (we already do something similar for programmatic changes).

I'll leave this issue open like this for this thought, but in the meantime I think adding distinctUntilChanged is the intended solution and not so much of a workaround. Does that work for you?

@Airblader Airblader added Comp: Core Core functionality of ngqp Priority: Medium Users can work without this for now, but this should be fixed soon-ish Status: Needs Design There is no clear API for this feature yet and one should be designed before work on ican be started and removed Status: Needs Triage We need to properly tag this issue labels Jun 15, 2020
@osahner
Copy link
Author

osahner commented Jun 15, 2020

Sure, just a small Note in the docs would be nice.

@Airblader
Copy link
Collaborator

Yes, a note in the documentation would definitely be a good idea as well. Feel free to submit a PR if you want :-)

@osahner
Copy link
Author

osahner commented Jun 15, 2020

😉 I will

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Comp: Core Core functionality of ngqp Priority: Medium Users can work without this for now, but this should be fixed soon-ish Status: Needs Design There is no clear API for this feature yet and one should be designed before work on ican be started Type: Bug Something that should work doesn't
Projects
None yet
Development

No branches or pull requests

2 participants