AddIgniteUIBlazor fix loading modules in settings overload, docs and test coverage - #312
Conversation
AddIgniteUIBlazor Docs, fix loading modules in settings overload in AddIgniteUIBlazorAddIgniteUIBlazor docs, fix loading modules in settings overload and test coverage
AddIgniteUIBlazor docs, fix loading modules in settings overload and test coverageAddIgniteUIBlazor fix loading modules in settings overload, docs and test coverage
There was a problem hiding this comment.
Pull request overview
Updates the AddIgniteUIBlazor DI extension docs and fixes how the settings-based overload composes ModulesToLoad, backed by new unit tests to prevent regressions in service registration and module preloading behavior.
Changes:
- Added XML documentation to
AddIgniteUIBlazoroverloads describing runtime registration and optional module preloading. - Fixed the settings overload to merge modules from the provided settings with the
paramsmodules (instead of replacing/clearing them). - Added unit tests covering service registration, module list composition/deduplication, and module
Registerinvocation behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/IgniteUI.Blazor.Tests/ServiceRegistrationTests.cs | Adds coverage for AddIgniteUIBlazor service registration and module list composition/registration behavior. |
| src/componentsBase/InfragisticsBlazorExtensions.cs | Adds docs and fixes module list merging behavior for the settings-based registration overload. |
Suppressed comments (1)
tests/IgniteUI.Blazor.Tests/ServiceRegistrationTests.cs:132
- This test resolves the scoped runtime from the root provider. To better reflect real-world Blazor DI (and to keep the module-registration assertion tied to the scoped lifetime), resolve the runtime from an explicit scope.
using var provider = Build(s => s.AddIgniteUIBlazor(typeof(FirstModule), typeof(SecondModule)));
var runtime = provider.GetRequiredService<IIgniteUIBlazor>();
Assert.Same(runtime, Assert.Single(FirstModule.Registrations));
Assert.Same(runtime, Assert.Single(SecondModule.Registrations));
}
| public static Microsoft.Extensions.DependencyInjection.IServiceCollection AddIgniteUIBlazor(this Microsoft.Extensions.DependencyInjection.IServiceCollection collection, | ||
| params Type[] modulesToLoad) | ||
| { |
There was a problem hiding this comment.
@copilot That'd be a bit out of scope for this PR, so attempt a fix in a separate PR against master
627e712 to
c27df78
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/componentsBase/InfragisticsBlazorExtensions.cs:85
settingsis already dereferenced on the previous line (new IgniteUIBlazorSettings(settings)), so the null-conditionalsettings?.ModulesToLoadis redundant and can misleadingly suggestnullis supported here. Consider usingsettings.ModulesToLoadfor clarity.
Type[] modules = [.. (settings?.ModulesToLoad ?? Enumerable.Empty<Type>())
.Concat(modulesToLoad ?? Enumerable.Empty<Type>())
.Distinct()];
|
@damyanpetev I've opened a new pull request, #313, to work on those changes. Once the pull request is ready, I'll request review from you. |
Delayed docs for
AddIgniteUIBlazorsince they should be payed special attention and there was a bug that was again discovered during #286 that's fixed and test coverage is a added since it was missing entirely.AddIgniteUIBlazor(settings)silently discarded any resource modules the settings already carried, so preloading through the settings object never worked unless the same list was repeated as params.The bug
Two adjacent lines in the settings overload:
WithModulesToLoadassigns unconditionally, so with no params thenullwins:The copy constructor deliberately preserving
ModulesToLoadis what makes this read as an oversight rather than a decision — one line puts the list in, the next takes it out.The fix
The params now add to what the settings carry instead of replacing it, de-duplicated and settings-first:
Tests
There was no coverage for
AddIgniteUIBlazororIgniteUIBlazorSettingsat all — the types appeared only in the TestBed'sProgram.csand the interop harness, never asserted.ServiceRegistrationTestsnow covers:IIgniteUIBlazorSettingsandIIgniteUIBlazor, and return the collection for chaining;ModulesToLoadnull;ForceJsonDataMarshalling) survive the copy;Register, once per scope — which is what preloading actually means, rather than just checking the list.Two of them fail against the old code, verified by reverting the fix.
Verification
dotnet buildclean; unit suite 891 passed, 0 failed.