Skip to content

Conversation

@linkdotnet
Copy link
Collaborator

This closes #1580

The basic idea here is to have a router that takes care of everything. There are a few open points. I will mark them here. We can discuss this next Friday, I just had some time to do the feature today and wanted to finish it as much as possible.

@linkdotnet linkdotnet requested a review from egil October 12, 2024 15:00
@linkdotnet linkdotnet force-pushed the navigation-properties branch 2 times, most recently from c0bfe13 to 97cff02 Compare October 13, 2024 09:20
@linkdotnet linkdotnet force-pushed the navigation-properties branch from c3daea1 to b2137f1 Compare November 3, 2024 10:00
@linkdotnet
Copy link
Collaborator Author

linkdotnet commented Nov 3, 2024

Just some paper-trail for our future selfs:
To make that work with bids of ASP itself,we need:

  • RouteTable - that does all the nice URL segment parsing for us
  • That depends on TreeRouter, which is internal for the Blazor case (see: #ifdef !COMPONENTS).
  • That objects has then again a dependency that is internal for us as well: UrlMatchingTree

Assumption here is that we don't need the TreeRouter in the first place and can solely rely on RouteTable.Route method or even alternatively the internal static ProcessParameters method.

RouteTable.Route takes the TreeRouter unfortunately - so we are stuck to the limbo described above. The static version already wants a RouteData object - so the "parsed" version where the segments are already parsed to a IReadOnlyDictionar<string, object> (aka the heavy lifting!).

While properly all of that (at least three reflection invocations plus some more setup to create RouteView, HttpContext*, ...) works, I am not sure if writing the code itself (aka this PR) is so much worse.

* The RouteContext that is used by the TreeRouter inside Route does want a HttpContext. I am not sure if I am missing something - but why does it want this in the first place? It is not even used inside that object - and gets just gets it passed in from somewhere. So we also might have to setup a HttpContext here (at least so that it doesn't throw the ArgumentNotNullException in its ctor).

CC: @egil
Not sure if I missed something here or went offtrack.

@linkdotnet linkdotnet force-pushed the navigation-properties branch from 640d486 to 46f5dfb Compare February 22, 2025 10:16
@linkdotnet linkdotnet force-pushed the navigation-properties branch from 46f5dfb to 6abd715 Compare March 8, 2025 10:32
@linkdotnet linkdotnet changed the base branch from main to v2 March 8, 2025 10:32
@linkdotnet
Copy link
Collaborator Author

@egil I rebased all the work on v2. Will make some other small changes to make it "ready"

@linkdotnet linkdotnet force-pushed the navigation-properties branch from 6abd715 to 46e7ef6 Compare March 8, 2025 10:35
@linkdotnet
Copy link
Collaborator Author

Okay done :D - now the implementation is using our renderers SetDirectParametersAsync.

That was a shortcoming that was easy to overcome with v2.

In any case many points are up to discussion. So we are not bored next Friday.

@linkdotnet linkdotnet force-pushed the navigation-properties branch from 46e7ef6 to 6ee39df Compare March 8, 2025 10:37
@linkdotnet linkdotnet force-pushed the navigation-properties branch 2 times, most recently from c0317cc to f73a3bd Compare July 1, 2025 20:09
@linkdotnet linkdotnet force-pushed the navigation-properties branch from f73a3bd to 5f8cb25 Compare August 30, 2025 11:39
@linkdotnet
Copy link
Collaborator Author

@egil I updated the branch. I am still very convinced that this approach is better than reflection-based (across 3 to 4 different objects).

A different perspective for v2: Even if we role initially with this and then use the reflection-based, we would at least enable something early on.

@egil
Copy link
Member

egil commented Aug 30, 2025

@egil I updated the branch. I am still very convinced that this approach is better than reflection-based (across 3 to 4 different objects).

A different perspective for v2: Even if we role initially with this and then use the reflection-based, we would at least enable something early on.

Ok, could there be something we can ask the Blazor team to change in the code to enable us to do this better. They seem open to suggestions :)

@linkdotnet
Copy link
Collaborator Author

linkdotnet commented Aug 30, 2025

@egil I updated the branch. I am still very convinced that this approach is better than reflection-based (across 3 to 4 different objects).
A different perspective for v2: Even if we role initially with this and then use the reflection-based, we would at least enable something early on.

Ok, could there be something we can ask the Blazor team to change in the code to enable us to do this better. They seem open to suggestions :)

Good point - I think making the objects in question pub-ternal (how they already have many types and EF does a lot o things).

EDIT: Alternatively, an easy wrapper that takes an url, a component and spits out a ParameterView would also be nice :D

@egil egil requested a review from Copilot September 14, 2025 16:02
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements a feature that allows bUnit's NavigationManager to automatically set component parameters based on route templates during navigation, addressing issue #1580.

Key changes:

  • Added router functionality that parses route templates and maps URL segments to component parameters
  • Integrated the router with the existing NavigationManager to trigger parameter updates on navigation
  • Added comprehensive test coverage for various routing scenarios

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
ComponentRouteParameterService.cs Core service that handles route template parsing and parameter mapping
BunitNavigationManager.cs Updated to integrate with the new route parameter service
BunitContext.cs Added tracking of rendered components for parameter updates
RouterTests.cs Comprehensive test suite covering various routing scenarios
RouterTests.Components.cs Test components with different route configurations
passing-parameters-to-components.md Documentation for the new routing parameter feature
CHANGELOG.md Added feature description to changelog

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

public ComponentFactoryCollection ComponentFactories { get; } = new();

/// <summary>
/// Gets the components that has been rendered by this <see cref="BunitContext"/>.
Copy link

Copilot AI Sep 14, 2025

Choose a reason for hiding this comment

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

The TODO comment should be replaced with proper documentation describing the purpose and usage of the ReturnedRenderedComponents property.

Suggested change
/// Gets the components that has been rendered by this <see cref="BunitContext"/>.
/// Gets the set of rendered components that have been returned by this test context.
/// This is used internally to track components created during testing, enabling proper disposal and management.

Copilot uses AI. Check for mistakes.
Comment on lines +59 to +62
if (templateSegments.Length > uriSegments.Length)
{
return [];
}
Copy link

Copilot AI Sep 14, 2025

Choose a reason for hiding this comment

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

This logic doesn't handle optional parameters correctly. A template with optional parameters could be longer than the URI but still be a valid match. Consider checking if the extra template segments are optional before returning an empty dictionary.

Copilot uses AI. Check for mistakes.
Comment on lines +108 to +112
private static object GetValue(string value, PropertyInfo property)
{
var underlyingType = Nullable.GetUnderlyingType(property.PropertyType) ?? property.PropertyType;
return Convert.ChangeType(value, underlyingType, CultureInfo.InvariantCulture);
}
Copy link

Copilot AI Sep 14, 2025

Choose a reason for hiding this comment

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

The Convert.ChangeType call can throw exceptions for invalid conversions. This method should handle conversion failures gracefully, especially for user-provided URL segments that might not be valid for the target type.

Suggested change
private static object GetValue(string value, PropertyInfo property)
{
var underlyingType = Nullable.GetUnderlyingType(property.PropertyType) ?? property.PropertyType;
return Convert.ChangeType(value, underlyingType, CultureInfo.InvariantCulture);
}
private static object? GetValue(string value, PropertyInfo property)
{
var underlyingType = Nullable.GetUnderlyingType(property.PropertyType) ?? property.PropertyType;
try
{
return Convert.ChangeType(value, underlyingType, CultureInfo.InvariantCulture);
}
catch (FormatException)
{
return GetDefaultValue(underlyingType);
}
catch (InvalidCastException)
{
return GetDefaultValue(underlyingType);
}
catch (OverflowException)
{
return GetDefaultValue(underlyingType);
}
}
private static object? GetDefaultValue(Type type)
{
// Return null for reference types and Nullable<T>, otherwise default(T)
if (!type.IsValueType || Nullable.GetUnderlyingType(type) != null)
return null;
return Activator.CreateInstance(type);
}

Copilot uses AI. Check for mistakes.
@linkdotnet linkdotnet closed this Sep 14, 2025
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.

Parameter not read from route

3 participants