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 for only 1 ng-container that populates after ngAfterViewInit is ran #489

Merged
merged 3 commits into from
Apr 25, 2018
Merged

Fix for only 1 ng-container that populates after ngAfterViewInit is ran #489

merged 3 commits into from
Apr 25, 2018

Conversation

xonuzofa
Copy link
Contributor

@xonuzofa xonuzofa commented Apr 20, 2018

Fixes and passes tests for the ng-option bug when the ng-option is populated after the ngAfterViewInit has ran.

@coveralls
Copy link

coveralls commented Apr 20, 2018

Coverage Status

Coverage decreased (-0.2%) to 93.092% when pulling a112c10 on ResolveIO:Fix-for-only-1-ng-option-async-data into 06a406c on ng-select:master.

@xonuzofa
Copy link
Contributor Author

Sorry didnt realize it would recheck it after I committed it, so I made a new PR. This is keeping the same if statement and just adding a >= instead of >. This passes all tests

@xonuzofa xonuzofa changed the title Fix that passes tests Fix for only 1 ng-container that populates after ngAfterViewInit is ran Apr 20, 2018
@anjmao
Copy link
Member

anjmao commented Apr 24, 2018

@xonuzofa Hi, could you check https://github.com/ResolveIO/ng-select/blob/315c02cce567726ce7d182c51a2617f0ba5b1b40/src/ng-select/ng-select.component.ts#L479 It should not be called if ng-option template is not used at all, but because it is QueryList it will have empty array even if [items] Input is used instead of ng-option.

@xonuzofa
Copy link
Contributor Author

Hi @anjmao. I have added to the demo page data sources. I have commented out the old way and added the new way under ng-select.component.ts.

To replicate this bug, use the old way, click the button on the last test in data sources and there is no options that ever get populated. The new way, the options get populated correctly.

This is because the data does not get populated before ngAfterViewInit, therefore the this.ngOptions.changes code never is ran.

To argue your point about the QueryList having an empty array and replacing items, the this._setItemsFromNgOptions() function only gets called if the items.length === 0 anyways, so you are really not hurting anything. Then the handleNgOptions and handleOptionChange functions only gets called once a change is detected by this.ngOptions.changes (all inside _setItemsFromNgOptions()).

So if no ng-option was ever used and items get populated, having subscribed to changes will never overwrite the items. As you can see, with the new fix, all other examples work as expected.

I believe this is the correct fix that needs to take place in order to fix this bug.

@@ -196,7 +196,13 @@ export class NgSelectComponent implements OnDestroy, OnChanges, AfterViewInit, C
}

ngAfterViewInit() {
if (this.ngOptions.length > 0 && this.items.length === 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, just remove unneeded comments, and I can merge it 👍

ngAfterViewInit() {
   if (this.items && this.items.length === 0) {
      this._setItemsFromNgOptions();
   }
}

// }

// New Way
if (this.items && this.items.length === 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what will happen when this.items are loaded asynchronously?

@xonuzofa
Copy link
Contributor Author

Test / comments have been removed. @varnastadeus the this._setItemsFromNgOptions() function only gets called if items exists and items.length === 0. In the first async test on data sources, it uses the items with | async and this makes items = null, so this._setItemsFromNgOptions() function never gets called. Again, it only calls if length is 0, and then it just sets the onchanges function to detect if any ng-options are added in later. It will only overwrite the items if it detects changes, which if you use items only, that will never happen.

@anjmao anjmao merged commit 47f2e59 into ng-select:master Apr 25, 2018
@xonuzofa xonuzofa deleted the Fix-for-only-1-ng-option-async-data branch April 25, 2018 16:25
jakemdunn pushed a commit to jakemdunn/ng-select that referenced this pull request Oct 16, 2018
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.

None yet

5 participants