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

<select> value is empty #32

Closed
luchsamapparat opened this issue Jan 17, 2018 · 14 comments
Closed

<select> value is empty #32

luchsamapparat opened this issue Jan 17, 2018 · 14 comments
Labels

Comments

@luchsamapparat
Copy link

Hi,

I have an issue with all select fields in my application which I didn't have earlier, but I'm not sure when the issue appeared first. I haven't managed to reproduce that issue in a clean project and I cannot publish my code, however I started debugging the issue and maybe someone can point me into the right direction on what I might do wrong.

First of all, I am aware of #23, however I debugged setViewValue and it uses the correct value.

The initial situation is that I have a form state which already has a value assigned to the field which is rendered as a select field.

The control state:

{
  "id": "airline.createRfp.proposalCurrencyLabor",
  "value": "USD",
  ...
}

The template:

<select [ngrxFormControlState]="controlState">
    <option  value="USD">USD</option>
    <option  value="EUR">EUR</option>
</select>

I'll try to explain the issue with screenshots I made while debugging:

First, setViewValue is called with the initial value (USD) but the optionsMap is still empty so the element's value is set to null.

image

Then the vlaue of the "USD" option is set to "0" via the ' NgrxSelectOption. This fixes the optionMap` problem above from what I understand.

image

Then setViewValue is called again with "USD" and because the optionMap now has the correct value, the select is rendered with the correct value

image

image

Now is the point were I suspect the problem. For whatever reason NgrxSelectMultipleOption's ngOnInit fires and sets the options value to "". Note: The select is not configured to allow multiple values! Maybe, because both NgrxSelectOption and NgrxSelectMultipleOption have the same selector?

image

Now, when the setViewValue method is called the third time around, selectedId is "0", which is correct, however the option's value has been changed back to the original "USD" value and not "0"

image

In the result, the select loses its value.

Any ideas?

@MrWolfZ
Copy link
Owner

MrWolfZ commented Jan 17, 2018

That behavior is indeed a bug. I should have a fix ready soon.

@MrWolfZ MrWolfZ added the bug label Jan 17, 2018
@MrWolfZ
Copy link
Owner

MrWolfZ commented Jan 17, 2018

I have just release version 2.1.2 which contains a bugfix for this issue. The ngOnInit from the NgrxSelectMultipleOption should now not mess with normal selects anymore.

@MrWolfZ MrWolfZ closed this as completed Jan 17, 2018
@luchsamapparat
Copy link
Author

Hi,

thanks for the quick reaction! Your fix indeed solved the issue that NgrxSelectMultipleOption resets the options value. However, after that it seems that Angular's change detection does the same leading to the same issue:

image

This happens after ...

NgrxSelectViewAdapter.prototype.setViewValue (nativeElement.value = null)
NgrxSelectOption.prototype.value (nativeElement.value = "0")
NgrxSelectViewAdapter.prototype.setViewValue (nativeElement.value = "0")

Basically, it's the same issue as before with the difference that this time it's the change detection that's resetting the option's value. :-/

BTW, ChangeDetection is not set to OnPush and the options are currently set statically as described in my original post.

Just out of curiosity, why is this mechanism of changing the option's value to an index even used? Why not use the values directly?

@MrWolfZ MrWolfZ reopened this Jan 18, 2018
@MrWolfZ
Copy link
Owner

MrWolfZ commented Jan 18, 2018

Sorry, in my hurry to fix this bug I only solved half of what you posted in your original post. I'll work on another fix, but that will probably have to wait until the weekend.

In regards to why the mapping is needed: raw HTML options only allow strings as values. Therefore, to support non-string values I had to come up with a different solution. Instead of storing the value directly in the option in HTML I store its ID and access the value through that. Angular solved this differently by introducing ngValue for non-string values. I thought I had found a better solution but maybe it simply doesn't work. However, I still hope that I can make my solution work correctly in all scenarios.

@MrWolfZ
Copy link
Owner

MrWolfZ commented Jan 20, 2018

@luchsamapparat I had a look at my and Angular's code and there shouldn't be any reason why the change detection would directly change the option element's value (the change detection will never touch any HTML elements directly, only though components and directives).

However, I have a theory. Do you by chance still import the @angular/forms ReactiveFormsModule? That one also has a directive that is unconditionally applied to all option elements (I originally copied the code from there for my implementation before switching to the different approach). As you can see in the code here it also binds the value attribute and again unconditionally sets the element's value. This would explain what you see.

Assuming that this theory is correct I am still not sure how to solve this. Obviously if you are not using reactive forms you could just remove the import. But some people are using a combination of both. Maybe the order of imports matters, i.e. if you import the ngrx-forms module after reactive forms it's directives might be applied later causing everything to work again.

If my theory is not right I am a bit at a loss. Can you please debug again and step into the property assignment operation that is highlighted in your screenshot? I expect that the assignment is actually calling a setter property so there might be some more logic in there.

@luchsamapparat
Copy link
Author

Hi @MrWolfZ,

thanks for spending your weekend to look into this. I was wondering why this issue appeared all of a sudden but didn't have the time to go through all the changes of the last weeks to determine the change that caused this issue in our application. However, your theory reminded me, that we indeed added an import not for ReactiveFormsModule but FormsModule from @angular/forms.

We needed a form control for date/time values and for that we created a custom form element, that combines a datepicker and timerpicker. This datetime component internally uses ngModel and ngModelChange to retrieve the values of the input elements and then propagates a combined value via the ControlValueAccessor interface in order to use it with ngrx-forms.

I'll debug the application, check your theory and will report my results in an hour.

Best regards,
Marvin

@luchsamapparat
Copy link
Author

Okay, I can already confirm that removing the FormsModule import and ngModel from the custom form element indeed solves the issue.

@luchsamapparat
Copy link
Author

luchsamapparat commented Jan 20, 2018

NgSelectOption._setElementValue is indeed called for each option after NgrxSelectOption.value was called for the same element, thereby resetting the element's value. And reordering the imports so that NgrxForms is imported after FormsModule solves the issue. Not what I call a solid solution, but at least a good workaround ;-)

Edit: I already tried to create a reproduction, however didn't have enough information to reproduce this issue. With this new information I can try again and publish it.

@MrWolfZ
Copy link
Owner

MrWolfZ commented Jan 20, 2018

Glad to hear that we found a workaround at least. However, I fear that the only proper solution is a pull request to @angular/forms to change the way their option code works. IMO if the option cannot be associated with a select (which they are already checking) the code shouldn't do anything at all, i.e. not change the HTML element's value. Maybe some other idea will come to my mind over the next few days, but for now I have nothing.

@luchsamapparat
Copy link
Author

I see. If something comes to mind and you want to try, I created a reproduction where you can test the issue: https://github.com/luchsamapparat/ngrx-select-bug

Thanks again for helping with the issue. If you ever come to Hamburg, contact me and I'll buy you a beer. :-)

@MrWolfZ
Copy link
Owner

MrWolfZ commented Jan 20, 2018

Thanks for the reproduction repo. I'll use it to verify whatever solution I come up with. I'll leave this issue open to track it.

Haven't been in Hamburg in ages (I'm from Berlin), but if I ever am I'll take you up on that offer :)

@MrWolfZ
Copy link
Owner

MrWolfZ commented Feb 3, 2018

As you can see above I have filed a PR with Angular to fix this issue properly. Let's see whether it gets accepted.

@MrWolfZ
Copy link
Owner

MrWolfZ commented Apr 15, 2018

:( no movement at all on that Angular PR. Doesn't seem like this will be fixed after all. At least you have a workaround. I'll add a note about this issue in the FAQ I am going to write. For now I'll close this issue.

@MrWolfZ MrWolfZ closed this as completed Apr 15, 2018
@migmolrod
Copy link

@MrWolfZ It seems the Angular team finally replied. Better late than never 😛
They find the change valid. But at this point it's so far from master Angular branch that they need the PR to be rebased. Any chance this can be done? If not, I could at least try to resend your PR, branching from latest develop branch so they can merge it (keyword: try. I've never contributed to such a huge project like Angular is)

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

No branches or pull requests

3 participants