fix(module:select): value no longer reset on datasource set

[anddrzejb]

🤔 This is a ...

• New feature
• Bug fix
• Site / documentation update
• Demo update
• Component style update
• Bundle size optimization
• Performance optimization
• Refactoring
• Code style optimization
• Test Case
• Branch merge
• Other (about what?)

🔗 Related issue link

Fixes #1719
Fixes #1905

💡 Background and solution

I found out that issue #1905 was basically caused by PR #1720. I did a deeper dive and I found out that the solution employed by @anranruye was causing additional re-render and resetting of the bound value. The problem was that when DataSource changes, then OnValueChanged is called which is trying to find current Value in the SelectOptionItems. If nothing is matching in SelectOptionItems, then the Value is set to default. The thing is, SelectOptionItems might not yet be populated with DataSource. Thus, I added populating of the SelectOptionItems if DataSource change is detected (and added a boolean _alredyRecreatedSelectOptions to prevent multiple recreation of SelectOptionItems in CreateDeleteSelectOptions() method. I also rolled back the changes of PR #1720.

I also added the tests to cover the issues.

📝 Changelog

Language	Changelog
🇺🇸 English
🇨🇳 Chinese

☑️ Self Check before Merge

⚠️ Please check all items below before review. ⚠️

• Doc is updated/provided or not needed
• Demo is updated/provided or not needed
• Changelog is provided or not needed

[github-actions]

[codecov]

Codecov Report

Merging #1906 (af8d541) into master (2478233) will increase coverage by 1.41%.
The diff coverage is 89.83%.

@@            Coverage Diff             @@
##           master    #1906      +/-   ##
==========================================
+ Coverage   22.51%   23.93%   +1.41%     
==========================================
  Files         481      499      +18     
  Lines       31326    23414    -7912     
  Branches        0      129     +129     
==========================================
- Hits         7053     5603    -1450     
+ Misses      24273    17808    -6465     
- Partials        0        3       +3     

Impacted Files	Coverage Δ
...ents/select/internal/DataSourceEqualityComparer.cs	70.00% <70.00%> (ø)
components/select/Select.razor.cs	49.04% <92.50%> (+12.13%)	⬆️
components/select/SelectOption.razor.cs	83.48% <100.00%> (+6.08%)	⬆️
components/core/Helpers/MemberPath/PathNode.cs	46.15% <0.00%> (-8.85%)	⬇️
components/core/Reflection/TypeDefined.cs	77.77% <0.00%> (-7.94%)	⬇️
components/input/InputGroup.razor.cs	87.50% <0.00%> (-6.95%)	⬇️
components/tabs/TabPane.razor	86.66% <0.00%> (-6.20%)	⬇️
components/core/Helpers/THelper.cs	40.00% <0.00%> (-4.45%)	⬇️
components/core/Extensions/JSRuntimeExtensions.cs	38.88% <0.00%> (-4.45%)	⬇️
...ponents/date-picker/types/DatePickerPlaceholder.cs	22.72% <0.00%> (-4.20%)	⬇️
... and 411 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2478233...af8d541. Read the comment docs.

[anranruye]

Notice that #1536 and #1719 are different. One is using DataSource approach, another one is using SelectOption approach. (I asked @ElderJames to reopen #1536, which is not actually solved by #1720) Please make sure you can solve both.

[anranruye]

I think there is indeed a bug when DataSource changes, as I mentioned in #1886

The second Select doesn't select any option, but the value is not set to null.

However, I can't see anything wrong with #1905. You are using both OnInitialized() and OnInitializedAsync() (why don't you also get the _model object after a delayed task?) so the page is rendered twice, before and after you get the datasource items. The first time, values of _model are not null, but the datasources are null, so the model values are set to null.

[anranruye]

However, I do get your expected result with 0.8.2 version(#1720 is merged since 0.8.3), I don't know why. Can you tell me what is the difference between putting the code in the property setter and putting the code in OnParametersSet method?

I guess the difference is, the code in property setter is using if (_isInitialized) which avoid to call OnValueChange for the first render, but both _optionsHasInitialized and _isInitialized are true when OnValueChange is called in OnParametersSet, so OnValueChange is called for every render.

As I mentioned, the calling order of property setters is not consistent. I still think the code should be written in OnParametersSet method.

[anddrzejb]

The issue was indeed with the fact that value was reset even during initialization (or rather during OnParametersSet() even if this was first time run after initialization). And you were right, this should not be run in setters. My mistake was that I just rolled back to working state and started making changes that would not break PR #1720 fix + would also fix #1905. On top of that, I was following the implementation of Value in AntInputComponentBase.

Your suggestion for loading the model after the delayed task would not work in my use case, because my real model has more than 2 select components. I tried. I would have to reload the model after every task or use other set of variables. Regardless, in my opinion it is not intuitive and should be fixed.

Anyway, please have a look at my latest commit. I also fixed DataSource change detection - now it also detects adding/removing (which is also reflected in the test changes & docs).

I also checked the issue you mentioned in the docs - it seems to me that antD has exactly the same behavior...

@ElderJames can you please remove #1536 from the list of issues this PR should solve? I incorrectly added it there and my privileges do not allow me to remove it by myself.

[ElderJames]

@ElderJames can you please remove #1536 from the list of issues this PR should solve? I incorrectly added it there and my privileges do not allow me to remove it by myself.

I can't remove it directly either, but modifying the content of the pull request does. Maybe this is a new mechanism in GitHub.

[anranruye]

before you reply, I tried to avoid the first call to OnValueChange and got the expected result. I think your code can get the same effect though I haven't download and run your code.

But when I try to solve the issue I mentioned above together(by call On ValueChanged when DataSource changes, I still avoid the first call in this case), things went wrong(the select components display empty selection). Can you try to solve this issue(value not reset to null when DataSource changes) together?

[anranruye]

Your use case makes sense and I also have some use case like that(however, I only have two Select in the same time and I get the model after the select DataSource).

In fact, I have another idea. I think we should
always remain the model value, no matter whether it has a corresponding select option or not. the model value should only be changed when DataSource changes(of course we may need to ignore the first(or also the second?) change when initializing) or the user select an option by ui. Changing the Value property by code only affect the ui, but this value will not be rewritten by the Select component even if it is not valid.

[anddrzejb]

@anranruye I have mixed feelings about your proposal. It seems to me a bit inconsistent. Your suggestion is to:

• not change the bound value when for example an element does not exist in the DataSource - as far as I can see this can happen only when an item was removed from the DataSource
• change the bound value when the Datasource changes - what about during initialization? What about if new DataSource actually has the current value?

What I do not like is the idea that a ui library takes a decision that should be left to the developer. Instead, I would actually advocate for additional parameter (an enum) that would allow the consumers to choose:

• reset value if not present in datasource (applies to situation when an item for the DataSource is removed or when the DataSource is changed)
• always reset value on DataSource change
• reset value on DataSource change if value not present in the DataSource

Or we never change (regardless) and tell the people to actually make their adjustment in OnDataSourceChange. Because my change now also triggers when an item is removed from the DataSource, that should cover the aforementioned scenarios as well as any new scenarios. We would just shift the responsibility to the consumers & possibly keep the Select component a bit simpler.

[anranruye]

@anddrzejb I think you are right, and your suggestions are both not bad choices(In fact, I am doing something like that).

But let me explain why my idea seems a bit inconsistent.
Consider two situations:

Select options are hard-coded at the client side. "1","2","3" are available. At the beginning, this works well. But as time goes on, the business develops, "4" and "5" are also valid values. Then some clients update, some clients not. When edit an item on a not updated client, we may get model value "4" from the server, but it is not in the data source("1", "2", "3"). Here we should decide to remain the value "4" or set the model value to null. I think still using "4" as the value is better, and for other situations where the model value(especially when the model is already in db) has a conflict with the select options, we should respect the model first.

Another situation is the typical province - city cooperating I showed above. Consider we have "A" province with its two cities "A1" and A2, and B province with B1 and B2. City field is not required. If we select A-A1 first, then change to
"B-?", we expect to send "B-null" to the server but the actual result is "B-A1". City is not set to null and even worse, A1 doesn't belong to B.

I like the idea that we never change, just let the user to make their own decision. But there are also some developers who are not against that we implement the most common situations. If we want to do this, then we need to consider the situations I mentioned above.

[anranruye]

Maybe we can follow these rules to cover the most common usage:

1. When change Value(by change the variable bound to Value parameter with C# code) only, do not modify the bound variable back even if it is not present in datasource, because this value is explicitly set by the user and we should respect this value.
2. When change Datasource only, if the bound variable value is not present in previous datasource and not present in the new datasource, do not change bound variable; if the bound variable value exists in previous datasource and not present in the new datasource, change bound variable to null/default.
3. When change Value and Datasource both, I'm not sure, I prefer the same behavior with 1 (?) for the same reason, the value is set explicitly by the user/developer.

[anranruye]

I don't think we can rely on the item count, and SelectOptionItems also contains added tags?

[anddrzejb]

I forgot about the tags. Indeed it does, but it can be simply fixed by subtracting what we have in AddedTags.
Count gives a basic check over the basic add/remove. It obviously won't work when a consumer adds & remove the same number of items from the DataSource. From what I can see, currently the whole sequence comparison in DataSource setter is useless anyway. We could just replace it with reference equality check and I think it would work the same (except probably faster).
But, we should probably keep the SequenceEqual check but not to check it against _dataSource but against a deep copy of the _dataSource. I think then we could safely rely on the change detection.

[anddrzejb]

Sorry, I think shallow copy should be good enough.

[anddrzejb]

My tests revealed that shallow copy wont cover all scenarios (given the DataSource is an IEnumerable<Persons>):

        //will NOT detect
        _persons[2].Name = newValue; 
...
        //will detect
        _persons[2] = new Person { Id = 3, Name = "new Person()"  }; 
...
        //will detect
        _persons.Remove(_persons.Last());
        _persons.Add(new Person { Id = 3, Name = "removed and added" });

But we cannot really make a deep copy ourselves (possible issues: cycle object graph, references to external objects, events). So, to cover all scenarios, the only other way I can think of is to allow the consumers to pass their own IEqualityComparer we could use in SequenceEqual() method during DataSource change detection - I would think these are going to be the advanced scenarios, but still available to the developers.

[anranruye]

It seems we are breaking this use-case: https://ant-design-blazor.gitee.io/zh-CN/components/select#components-select-demo-TrackItemChanges. Now a custom IEqualityComparer is necessary to track item changes.

[anddrzejb]

Thanks, I will have a look.

I also experimented with IEqualityComparer but I think it won't really help us here, because in case of changes to an item of the DataSource we still are making changes to the same object. So the comparer compares an object to itself. Now I start to think that we should expose our own interface/base class for DataSource that will have a Clone virtual method, which will do a shallow copy by default but with ability to overload it, so the consumers can create a deep copy implementation. What do you think?

[anranruye]

Just call CreateDeleteSelectOptions when IgnoreItemChanges is false. Then shallow copy should be enough.

[anddrzejb]

I guess here we should as @ElderJames for his opinion. To be honest I think I prefer my last proposal - shift the responsibility to the consumer, do not force the behavior on the consumer. So basically, follow your proposal in 1 & 3. Which in turn would also mean that we apply same logic across the board - we don't change in 2 either.

[anranruye]

I think you are right. We should apply same logic for all situations and that will make everything simple.

If we decide to do this, then there comes an issue: how should we use DefaultValue and DefaultActiveFirstItem?

[anranruye]

I don't think we should change the value inside DataSource property. As I mentioned, the order to set Value and DataSource depends on user hand-writing. And this is actually unnecessary.

[anddrzejb]

If our proposal of removing value changes on DataSource changes gets accepted, then this will most likely be removed from the setter.

[anddrzejb]

@ElderJames What do you think about our changes proposal to the value? I do not want to start doing something that you might possibly reject later on.

@anranruye Regarding default behavior - I believe that default value should kick in only during initialization or when clear button is clicked. If we keep the value even if it does not exist in DataSource, we should visually reflect it by not showing anything. And possibly describe in detail the Select's behavior in the docs.

[anranruye]

I do know that DefaultValue/DefaultActiveFirstOption is used only once during initialization. That's the problem. I think people who use DefaultActiveFirstOption want to also see the first option is selected after DataSource changes(and the previous value is no longer valid). But this break the rule that we don't change the Value.

To be honest, I don't like DefaultValue/DefaultActiveFirstOption at all(and I will never use them). If we want the users to take care the Value which they want, as we discussed, then they should manully set their expected value to Value property, rather than DefaultValue property.

[anddrzejb]

I wholeheartedly agree with you. DefaultValue does seems to me like an overkill, especially that it is anyway loading Value with DefaultValue. I do however see an appeal in DefaultActiveFirstOption. Still, I think we should apply these only during initialization and on clear. Otherwise, keep the original values. Regarding the people who would be negatively impacted by our proposed change - I think we should strive to make the library to behave consistently.

[anranruye]

it is anyway loading Value with DefaultValue

Though our docs says that, it is not true. DefaultValue is only applied when the value is not present in the DataSource. Otherwise, Value property is used. #1872 was opened to try to implement this behavior. I talked about this with @ElderJames, and we think the current behavior is also ok, so we should avoid to impact users' existed code. I told the author of #1872 our decision, then he closed that pr.

I think we should apply these only during initialization and on clear

Basiclly I agree.

The doc doesn't mention "Clear" but it mentioned "Reset button within Forms", do you think "Clear" and "Reset" are the same? I think DefaultValue can be used when "Clear", but may be not when "Reset".

And I noticed that currently DefaultValue is not used when clear(it is used when the value is not present in the DataSource and AllowClear is false).

By the way, @ElderJames agrees with us. We should change the value only when the users select an option or click clear button.

[anddrzejb]

@anranruye , @ElderJames My latest commit 1a67ca3 this time actually fixes #1536. Changes in the commit:

• Better detection of changes made to DataSource - using single (for primitive types in the DataSource) or double (for reference types) shallow copy. It is achieved using reflection - but to improve performance, the MemberwiseClone method is fetched only once. Also, shallow copy is done only if change detection discovers differences. Otherwise, I do not think we should have any performance hit here - the SequenceEqual() was there before.
• This change also includes new [Parameter] DataSourceEqualityComparer that can be passed to the component for more advanced scenarios. I believe this change also allows to avoid rebuilding of the SelectOptionItems on every OnParametersSet() execution. Currently it is a [Parameter] but to be honest I would rather this be a public property accessible through instance (same logic as with Form) - this is for performance reasons. We should avoid non-primitives in [Parameter]. Also, I think this is an advanced scenario, so it should not be a problem for the consumers.
• New tests for change detection:
  • when item in the DataSource is replaced (datasource[1] = new Item())
  • when item's label is changed in the DataSource (datasource[1].Label = "Changed label")
• Fixes to Immediately Changing Value after item is modified in Select #1536: the problem here was (and kind-of still is) that when both Value & new select option is added at the same time, first what gets set is the Value. Because the SelectOption is not yet added (first parent renders then its children), the newly added Value is reset do default. Until we implement what we discussed here (that is - do not change the Value if it does not exist in the DataSource or SelectOption) the LastValueBeforeReset is used to actually revert to value that was existing before reset, but only if new SelectOption is added.
• New tests to support issue Immediately Changing Value after item is modified in Select #1536 resolution (for default & multiple scenarios)
• New tests to support checking that value does not reset on DataSource changes. 4 out of 6 tests are currently commented out, because they are not passing now. I decided to leave them for now, so hopefully the can be reinstated by anyone who is going to implement what we discussed in this PR.

Edit:
Commit 6bc7bcd adds test that covers scenario from issue #1207. This test however requires net6 to run.

[anranruye]

As I stated before, Reset method in Select component does exactly the same as Clear button:

So at this point there is no support for different behavior depending on clear or reset. As far as I understand, we were discussing an independent logic for clear and for reset.

Yes, we should make ClearSelectedAsync do the "clear" job and add new method(s) for "reset".

Regarding TItemValue being not nullable - again, I would advocate for consistency.

I don't like Inconsistency, either.

In case of mentioned #1587, shouldn't we always try to match entered value with existing TItemValue? I believe we should.

I think so.

Of course, if there is a select option that is equal to default(TItemValue) then the AllowClear should probably be ignored. Or otherwise it will work like reset with DefaultValue set to default(TItemValue).

That's the problem. Again, I want to emphasize, "clear" is "clear", it's not "reset" or anything else. for Input/InputNumber/TextArea, it's ok to set the value to 0 (for not nullable types). but this is not true for Select component. for not nullable types, anything we do will not be "clear".

The case is, C# has a different type system with Js. Js variables are always nullable. Js ui technologies don't have this issue, but here we have. My suggestion is to forbid "clear" for not nullable types.

When TItemValue is nullable, we actually call for type default. Should we break that and offer a different behavior for non-nullable?

Yes, I think we should break this for non-nullable types. My answer is to forbid "clear" for not nullable types. Instead, as I mentioned above, we can display a independent reset button.

@ElderJames doesn't decide yet. If you agree, maybe he will agree too.

[anddrzejb]

I think we agree. What I would add is a compiler warning when non-nullable type is used that would contain a link to detailed explanation of the problem to the docs.

[anranruye]

When use default DataSourceEqualityComparer and TItem is not primitive, _dataSourceHasChanged is always true and OnDataSourceChanged will be triggered.

[anddrzejb]

Please check commit af8d541 - I added internal DateSourceEqualityComparer that is comparing only values of label & value properties of the DataSource. Also, I included a test to cover the scenario.

[ElderJames]

Yes, I think we should break this for non-nullable types. My answer is to forbid "clear" for not nullable types. Instead, as I mentioned above, we can display a independent reset button.

I agree with you.
However, if the user still needs Clear, I suggest adding another property, such as ValueOnClear, to allow the user to specify which value to change.

[anranruye]

@ElderJames Ok. Though this will confuse the concept of "clear" and will bring inconsistency(we should always set the value to null for nullable types), it meets the actual usage habits of users.

[ElderJames]

@anddrzejb @anranruye Thank you for the great works.

[anddrzejb]

@ElderJames @anranruye Thanks for a fruitful discussion. I hope we will manage to implement the proposed changes.