Skip to content

[Mvc] Improved route value transformer #21481

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 7 commits into from
Jul 22, 2020

Conversation

rynowak
Copy link
Member

@rynowak rynowak commented May 4, 2020

See: #21471

This change just include the product-side changes and just for
controllers. This addresses most the major gaps our partners have found
with dynamic route value transformers. Doesn't include anything
order-related.

If we're happy with this then I can add some tests as well as make the same change for pages.

@rynowak rynowak requested review from pranavkm, JamesNK and javiercn May 4, 2020 22:12
@ghost ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label May 4, 2020
@rynowak
Copy link
Member Author

rynowak commented May 4, 2020

@JamesNK @pranavkm @javiercn - what do you think of this? read: #21471 first.

@rynowak
Copy link
Member Author

rynowak commented May 4, 2020

One other alternative I thought of - we could get rid of State as a property and make this use DI or Type activation - and then we use type activation if you pass a state value.

The problem is we require the use of DI here in 3.1 - supporting both adds more choices and more to explain.

@javiercn javiercn force-pushed the rynowak/transform-and-roll-out branch from 508a17d to e7a6a70 Compare July 21, 2020 19:54
@javiercn javiercn marked this pull request as ready for review July 21, 2020 20:08
@javiercn javiercn changed the title Prototype of improved route value transformer [Mvc] Improved route value transformer Jul 21, 2020
rynowak and others added 3 commits July 21, 2020 13:18
See: #21471

This change just include the product-side changes and just for
controllers. This addresses most the major gaps our partners have found
with dynamic route value transformers. Doesn't include anything
order-related.
@javiercn javiercn force-pushed the rynowak/transform-and-roll-out branch from 32778e3 to fe7dc75 Compare July 21, 2020 20:19
@rynowak rynowak requested a review from dougbu as a code owner July 21, 2020 20:19
@javiercn javiercn changed the base branch from master to release/5.0-preview8 July 21, 2020 20:19
@dougbu dougbu removed their request for review July 21, 2020 20:22
Copy link
Contributor

@pranavkm pranavkm left a comment

Choose a reason for hiding this comment

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

This looks great. @JamesNK could you have a look when you get a chance?

@javiercn
Copy link
Member

🆙📅

Added a check for the state not being null and matching unit tests.

Cleaned up error messages. Thanks @JamesNK, I totally overlooked the content.

Co-authored-by: James Newton-King <[email protected]>
@javiercn
Copy link
Member

@mkArtakMSFT Can you squash and merge this PR with the following title and comment?

[Mvc][Fixes #21471] Improvements to DynamicRouteValueTransformer

* Provides the ability to pass context to a given DynamicRouteValueTransformer on a per route basis.
* Requires DynamicRouteValueTransformer instances with associated state to be registered as transient.
* Enables filtering based on the results of selected endpoints by MVC.

@mkArtakMSFT mkArtakMSFT merged commit 6f37e8a into release/5.0-preview8 Jul 22, 2020
@mkArtakMSFT mkArtakMSFT deleted the rynowak/transform-and-roll-out branch July 22, 2020 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants