Skip to content

Conversation

@vernou
Copy link
Member

@vernou vernou commented Oct 30, 2024

Description

Before this PR, the value is coerced after the search. But the search can be delayed, which also delays the coercing of value :
old_behavior

After this PR, the value is coerced immediately :
new_behavior

Fixes #9420

How Has This Been Tested?

I added test.
I checked the doc page

Type of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation (fix or improvement to the website or code docs)

Checklist

  • The PR is submitted to the correct branch (dev).
  • My code follows the code style of this project.
  • I've added relevant tests.

@github-actions github-actions bot added the breaking change This change will require consumer code updates label Oct 30, 2024
@vernou vernou self-assigned this Oct 30, 2024
@ScarletKuro
Copy link
Member

If you will work on this, pls update your branch as we added nullable annotation to this component

@vernou vernou force-pushed the 9420-autocomplete-debounce branch from 18b6b23 to c3ae70a Compare November 3, 2024 23:30
@codecov
Copy link

codecov bot commented Nov 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.24%. Comparing base (aefe1e7) to head (06f303d).
Report is 19 commits behind head on dev.

Additional details and impacted files
@@           Coverage Diff           @@
##              dev   #10138   +/-   ##
=======================================
  Coverage   91.24%   91.24%           
=======================================
  Files         411      411           
  Lines       12495    12495           
  Branches     2434     2434           
=======================================
  Hits        11401    11401           
  Misses        552      552           
  Partials      542      542           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@vernou
Copy link
Member Author

vernou commented Nov 4, 2024

If you will work on this, pls update your branch as we added nullable annotation to this component

Done.


As this PR has breaking change, I want do more test before.

@vernou vernou marked this pull request as ready for review November 4, 2024 17:50
@sonarqubecloud
Copy link

sonarqubecloud bot commented Nov 5, 2024

@ScarletKuro ScarletKuro requested a review from henon November 5, 2024 12:43
@henon
Copy link
Collaborator

henon commented Nov 6, 2024

As this PR has breaking change, I want do more test before.

@vernou Do you still want to do more tests or can this be merged?

@vernou
Copy link
Member Author

vernou commented Nov 6, 2024

@vernou No, I finish to test. This is ready to be merged.

@henon
Copy link
Collaborator

henon commented Nov 6, 2024

If this fixes #9420 and that is considered a bug then I would say that this is not a breaking change per se.

@vernou
Copy link
Member Author

vernou commented Nov 6, 2024

@henon , the PR modify the behavior. It fixes the bug is a side effect.


The initial problem in #9420 was :

The value is coerced only if the search return 0 element, so the value is coerced after search, so after the debounce interval. But some event like OnBlur stop the debounce timer.

I see two solutions :

  1. When Immediate is enable, then the value is coerced immediately. Would be the expected behavior, but it's a breaking change.
  2. When the debounce timer is stoped, the value is coerced. Keep the weird behavior, but no breaking change.

As the modification will be integrated in v8, it's the good time to implement the solution 1.

@danielchalmers danielchalmers removed their request for review November 11, 2024 03:18
@vernou
Copy link
Member Author

vernou commented Nov 12, 2024

Ping @henon

@henon henon changed the title MudAutocomplete : Coerce value immediately MudAutocomplete: Coerce value immediately Nov 12, 2024
@henon henon merged commit c8529ab into MudBlazor:dev Nov 12, 2024
5 checks passed
@henon
Copy link
Collaborator

henon commented Nov 12, 2024

I agree that it is a good time to do solution 1, adding this to the v8 migration guide. Thanks @vernou

@henon henon mentioned this pull request Nov 12, 2024
<MudAutocomplete id="autocompleteLabelTest" T="string" Label="US States"
@bind-Value="Value" ResetValueOnEmptyText="ResetValueOnEmptyText"
SearchFunc="@SearchStates" DebounceInterval="DebounceInterval"
<MudAutocomplete id="autocompleteLabelTest" T="string?" Label="US States"
Copy link
Contributor

Choose a reason for hiding this comment

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

My IDE complains

The typeof operator cannot be used on a nullable reference type. (CS8639)

Copy link
Member

@ScarletKuro ScarletKuro Nov 13, 2024

Choose a reason for hiding this comment

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

Yeah, that's a bug dotnet/razor#8692. Razor generator breaks when you use ? with generic in razor syntax, which is why I added CS8669 as NoWarn. Seems like it a similar warring. But I think in that case you can revert the T="string?" to T="string" as the MudAutocomplete already specifies the value as T? rather than adding a new NoWarn.
It's pretty sad to see that the razor plays not really nicely with nullable

@vernou vernou deleted the 9420-autocomplete-debounce branch November 13, 2024 20:41
LLauter pushed a commit to cannellamedia/MudBlazor that referenced this pull request Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change This change will require consumer code updates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MudAutocomplete "Required" valiadtion failes when DebounceInterval is high enough and SearchFunc is slow.

4 participants