Skip to content

[Blazor] SSR unified routing #49622

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 20 commits into from
Aug 3, 2023
Merged

[Blazor] SSR unified routing #49622

merged 20 commits into from
Aug 3, 2023

Conversation

javiercn
Copy link
Member

@javiercn javiercn commented Jul 25, 2023

  • Unifies the routing implementation between ASP.NET Core routing and the Blazor interactive router.
  • Adds support for:
    • Complex segments.
    • Default values.
    • All the built-in constraints in ASP.NET Core.
  • Enables the interactive router to reuse the results from the ASP.NET Core router.
  • Updates the results of the ASP.NET Core router to match the specific Blazor interactive router quirks.
    • Adding null for all the unused parameters in a given route group for a given handler.
    • Adding null for all optional parameters that were not provided.
    • Converting some constraint values to their "native" type.
      • For example, a parameter with an int constraint gets converted to an integer from the parsed string.
  • Includes an extensibility point to support third-party routers via dynamic routing.

Meaningful behavior changes are:

  • Optional parameters won't throw if there are non-optional segments after them, the optionality will just be ignored.
  • Matching constrained catch-alls won't match on a per segment basis. This behavior was different in Blazor routing and is really incompatible with the ASP.NET behavior, so we rather standardize on the ASP.NET behavior.
    • To provide a concrete example, if you had /{*catchall:int} and the path /1/2/3/4/5, it would match on Blazor, but not on ASP.NET.

Implementation notes:

  • I've used TreeRouter which is the older implementation of the routing algorithm before we had the DFA, as it's a simpler and equally compatible/conformant implementation.
  • I've used #ifdefs around most of the code in routing to keep the change as simple as possible and avoid deviation in the future.
  • I've avoided using RouteTemplate and used RoutePattern directly as that's the most modern approach (RouteTemplate is legacy from the ASP.NET times)
  • I've copied the resources dictionary from routing to avoid adding extra resources that are not used.

@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Jul 25, 2023
@javiercn javiercn force-pushed the javiercn/unified-blazor-routing branch from 66fbf39 to 582c7af Compare July 26, 2023 13:56
@javiercn javiercn changed the title Javiercn/unified blazor routing [Blazor] SSR unified routing Jul 26, 2023
@javiercn javiercn marked this pull request as ready for review July 28, 2023 16:27
RouteValueDictionary values,
RouteDirection routeDirection)
#else
RouteValueDictionary values)
#endif
Copy link
Member

Choose a reason for hiding this comment

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

Who would you say is most responsible for maintaining this? I don't recall ever touching this code myself so am not clear on what level of maintainability is required here and what is the likelihood of nontrivial work happening in this area ever again.

This level of intricacy of #if over dozens of method signatures is not something I'd normally consider the way we do things, and is definitely suggestive that we've picked the wrong abstraction and should find a way to rework it. Clearly it would be better if the shared parts could be factored into a self-contained common package with suitable abstractions instead of the many #ifs.

However if you're taking the position that (1) this doesn't impact any other team, this is only ours to maintain and (2) there's no reason to think this is going to be developed any further in the forseeable future, then perhaps it's rational to accept the technical debt here (on the basis that we think nothing will ever force us to make repayments).

Copy link
Member Author

Choose a reason for hiding this comment

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

@SteveSandersonMS that would be me.

It's not likely for significant work to happen in this area (maybe we add an extra constraint, but that's it) and we can do the refactor in 9.0 if we need to.

I'm already thinking of reworking the constraints into something more modern and suitable for both, Blazor and ASP.NET Core, as they are legacy from the old routing times (if you see, they take an IRouter instance, which predates endpoint routing, but its there for legacy/compatibility). That said, it would be work for 9.0

@JamesNK
Copy link
Member

JamesNK commented Jul 31, 2023

Meaningful behavior changes are:

Are these changes to Blazor? Does ASP.NET Core server routing change at all?

@javiercn
Copy link
Member Author

Are these changes to Blazor? Does ASP.NET Core server routing change at all?

Yes, these changes only affect Blazor. ASP.NET Core routing remains exactly the same. The changes essentially align Blazor routing with ASP.NET Core routing.

@JamesNK
Copy link
Member

JamesNK commented Jul 31, 2023

If this merges, then the route highlighting rules for Blazor will need to be updated. Right now, it's customized for features that Blazor supports.

@javiercn
Copy link
Member Author

javiercn commented Jul 31, 2023

@JamesNK

If this merges, then the route highlighting rules for Blazor will need to be updated. Right now, it's customized for features that Blazor supports.

Is there a quick way that we can force it to match the ASP.NET language? I've perused through the code, but it is not clear to me how RouteOptions are selected

@JamesNK
Copy link
Member

JamesNK commented Aug 1, 2023

@JamesNK

If this merges, then the route highlighting rules for Blazor will need to be updated. Right now, it's customized for features that Blazor supports.

Is there a quick way that we can force it to match the ASP.NET language? I've perused through the code, but it is not clear to me how RouteOptions are selected

Yes! I designed it to be easy to change. Check out RoutePatternOptions.cs.

If component routes are the same as regular routes, then it's just a matter of updating ComponentRoute options to match DefaultRoute options. e.g.

    public static readonly RoutePatternOptions ComponentRoute = new RoutePatternOptions
    {
        SupportComplexSegments = true,
        SupportDefaultValues = true,
        SupportTwoAsteriskCatchAll = true,
    };

@javiercn javiercn force-pushed the javiercn/unified-blazor-routing branch from dcad98e to c0dbda2 Compare August 2, 2023 13:42
@javiercn
Copy link
Member Author

javiercn commented Aug 2, 2023

@JamesNK thanks!

I've updated the embedded language to match the default route semantics.

},
},
});
}
Copy link
Member

Choose a reason for hiding this comment

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

Do we definitely have to duplicate all this? It does make it quite difficult to maintain if we have many copies and don't even track which ones are applicable to which test.

Copy link
Member Author

Choose a reason for hiding this comment

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

Other than the suppress enhanced navigation, I believe we do. We can put it in a separate component if we want to.

The reason I chose to create a separate startup and set of tests is because I wanted to validate that we were able to prerender, reuse the results from asp.net core routing and then start the app in interactive mode (so that the router did the parsing) and made sure that worked, akin to Blazor Server but with the new APIs.

@SteveSandersonMS SteveSandersonMS self-requested a review August 3, 2023 14:01
Copy link
Member

@SteveSandersonMS SteveSandersonMS left a comment

Choose a reason for hiding this comment

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

Approving now to make sure you're unblocked.

I'm happy to take your word for the maintainability impact on the existing routing code. Clearly it's becoming a lot more complex with this and it would be good to have a path to removing the non-Components code paths. Since we are about to do a LTS, do you think we can realistically mark obsolete the non-Components code paths now, so it can all be removed in .NET 9? If not I think it would have to survive until .NET 11 based on normal rules.

Remaining thoughts:

  • I've posted a number of comments about the E2E tests. If those are valid points (e.g., would make sense for these tests to be about BasicTestApp and run on WebAssembly too) then it would definitely be good to address that, but if it has to be done as a follow-up in RC2/RTM that would be OK. I hope we don't just end up ignoring it though.
  • I'm assuming this doesn't meaningfully increase the default Blazor WebAssembly app size due to the much richer routing implementation. If it adds a few KB I'm sure we'll have no trouble with that, and I guess that's likely all it is. However if somehow this causes a lot more core libraries to be pulled in and we grow by tens of KB we should reconsider.

Is that OK?

@javiercn
Copy link
Member Author

javiercn commented Aug 3, 2023

@SteveSandersonMS that sounds good to me.

@javiercn
Copy link
Member Author

javiercn commented Aug 3, 2023

I'm happy to take your word for the maintainability impact on the existing routing code. Clearly it's becoming a lot more complex with this and it would be good to have a path to removing the non-Components code paths. Since we are about to do a LTS, do you think we can realistically mark obsolete the non-Components code paths now, so it can all be removed in .NET 9? If not I think it would have to survive until .NET 11 based on normal rules.

I think we could obsolete all the Tree matching stuff. We can discuss in more detail, but I've filed an issue here

@javiercn
Copy link
Member Author

javiercn commented Aug 3, 2023

#49084

@czirok
Copy link

czirok commented Aug 9, 2023

Hello @javiercn

Could you please tell me where I can find this:

  • Includes an extensibility point to support third-party routers via dynamic routing.

@ghost
Copy link

ghost commented Aug 9, 2023

Hi @czirok. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants