MudBlazor / MudBlazor

Blazor Component Library based on Material design with an emphasis on ease of use. Mainly written in C# with Javascript kept to a bare minimum it empowers .NET developers to easily debug it if needed.
http://mudblazor.com
MIT License
7.22k stars 1.18k forks source link

Better Illegal Razor Parameter Runtime Detection for v7 #8777

Closed henon closed 3 weeks ago

henon commented 3 weeks ago

Description

Follow-up to Illegal Razor Parameter Runtime Detection for v7 #8771

I noticed that the detection didn't work for generic types. After fixing that the detector finds at least 26 more illegal usages in our docs and tests. Naturally this PR fails until I fix the old params.

I think we probably should not remove this detector, ever. It is just too useful. We can use it to detect illegal usage of obsolete API in our docs and tests when we enter the next stable development phase. Of course then we'd change it to opt-in instead of opt-out.

How Has This Been Tested?

unit

Type of Changes

Checklist

codecov[bot] commented 3 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 90.10%. Comparing base (28bc599) to head (51da8f6). Report is 103 commits behind head on dev.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## dev #8777 +/- ## ========================================== + Coverage 89.82% 90.10% +0.27% ========================================== Files 412 419 +7 Lines 11878 12183 +305 Branches 2364 2399 +35 ========================================== + Hits 10670 10978 +308 + Misses 681 661 -20 - Partials 527 544 +17 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

henon commented 3 weeks ago

All errors fixed. Ready to merge

ScarletKuro commented 3 weeks ago

I think we probably should not remove this detector, ever.

Not sure about having it permanent as the reflection hurts performance

Also posting code in case we want to lower amount of ifs.

Code ```C# protected void DetectIllegalRazorParametersV7() { var illegalParameters = new Dictionary { { typeof(MudBadge), new[] { "Bottom", "Left", "Start" } }, { typeof(MudProgressCircular), new[] { "Minimum", "Maximum" } }, { typeof(MudProgressLinear), new[] { "Minimum", "Maximum" } }, { typeof(MudRadio<>), new[] { "Option" } }, { typeof(MudFab), new[] { "Icon" } }, { typeof(MudCheckBox<>), new[] { "Checked" } }, { typeof(MudSwitch<>), new[] { "Checked" } }, { typeof(MudPopover), new[] { "Direction", "OffsetX", "OffsetY" } }, { typeof(MudAutocomplete<>), new[] { "Direction", "OffsetX", "OffsetY" } }, { typeof(MudSelect<>), new[] { "Direction", "OffsetX", "OffsetY" } }, { typeof(MudToggleGroup<>), new[] { "Outline" } }, { typeof(MudAvatar), new[] { "Image", "Alt" } }, { typeof(MudSlider<>), new[] { "Text" } }, { typeof(MudRadioGroup<>), new[] { "SelectedOption", "SelectedOptionChanged" } }, { typeof(MudSwipeArea), new[] { "OnSwipe" } }, { typeof(MudChip<>), new[] { "Avatar", "AvatarClass" } }, { typeof(MudChipSet<>), new[] { "Filter", "MultiSelection", "Mandatory", "SelectedChip", "SelectedChipChanged", "SelectedChips", "SelectedChipsChanged" } }, { typeof(MudList<>), new[] { "SelectedItem", "SelectedItemChanged", "Clickable", "Avatar", "AvatarClass" } }, { typeof(MudListItem<>), new[] { "SelectedItem", "SelectedItemChanged", "Clickable", "Avatar", "AvatarClass" } }, { typeof(MudTreeView<>), new[] { "CanActivate", "CanHover", "CanSelect", "ActivatedValue", "ActivatedValueChanged", "Multiselection", "Activated", "ExpandedIcon", "SelectedItem" } }, { typeof(MudTreeViewItem<>), new[] { "CanActivate", "CanHover", "CanSelect", "ActivatedValue", "ActivatedValueChanged", "Multiselection", "Activated", "ExpandedIcon", "SelectedItem" } }, { typeof(MudMenu), new[] { "Link", "Target", "HtmlTag", "ButtonType" } }, { typeof(MudMenuItem), new[] { "Link", "Target", "HtmlTag", "ButtonType" } }, }; foreach (var parameter in UserAttributes.Keys) { foreach (var entry in illegalParameters) { if (MatchTypes(entry.Key)) { if (entry.Value.Contains(parameter)) { NotifyIllegalParameter(parameter); break; } } } // Additional illegal parameters not mapped to specific types var additionalIllegalParameters = new[] { "UnCheckedColor", "Command", "CommandParameter", "IsEnabled", "ClassAction", "InputIcon", "InputVariant", "AllowKeyboardInput", "ClassActions", "DisableRipple", "DisableGutters", "DisablePadding", "DisableElevation", "DisableUnderLine", "DisableRowsPerPage", "Link", "Delayed", "AlertTextPosition", "ShowDelimiters", "DelimitersColor", "DrawerWidth", "DrawerHeightTop", "DrawerHeightBottom", "AppbarMinHeight", "ClassBackground", "Cancelled", "ClassContent", "IsExpanded", "IsExpandedChanged", "IsInitiallyExpanded", "InitiallyExpanded", "RightAlignSmall", "IsExpandable" }; if (additionalIllegalParameters.Contains(parameter)) { NotifyIllegalParameter(parameter); } } } internal bool MatchTypes(params Type[] types) { var self = GetType().IsGenericType ? GetType().GetGenericTypeDefinition() : GetType(); return types.Any(type => self == type); } [ExcludeFromCodeCoverage] private void NotifyIllegalParameter(string parameter) { throw new ArgumentException($"Illegal parameter '{parameter}' in component of type '{GetType().Name}'. This was removed in v7.0.0, see Migration Guide for more info https://github.com/MudBlazor/MudBlazor/issues/8447"); } ```
henon commented 3 weeks ago

Not sure about having it permanent as the reflection hurts performance

With active opt-in the cost would be a single if

henon commented 3 weeks ago

wouldn't this perform a linear search?

      // Additional illegal parameters not mapped to specific types
      var additionalIllegalParameters = new[]
      {
          "UnCheckedColor", "Command", "CommandParameter", "IsEnabled", "ClassAction", "InputIcon", "InputVariant",
          "AllowKeyboardInput", "ClassActions", "DisableRipple", "DisableGutters", "DisablePadding", "DisableElevation",
          "DisableUnderLine", "DisableRowsPerPage", "Link", "Delayed", "AlertTextPosition", "ShowDelimiters",
          "DelimitersColor", "DrawerWidth", "DrawerHeightTop", "DrawerHeightBottom", "AppbarMinHeight", "ClassBackground",
          "Cancelled", "ClassContent", "IsExpanded", "IsExpandedChanged", "IsInitiallyExpanded", "InitiallyExpanded",
          "RightAlignSmall", "IsExpandable"
      };

      if (additionalIllegalParameters.Contains(parameter))
      {
          NotifyIllegalParameter(parameter);
      }
ScarletKuro commented 3 weeks ago

With active opt-in the cost would be a single if

oh, right