Skip to content

Progressively enhanced form posts #48939

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 10 commits into from
Jun 21, 2023

Conversation

SteveSandersonMS
Copy link
Member

@SteveSandersonMS SteveSandersonMS commented Jun 21, 2023

Implements #48762

This extends the progressively enhanced nav feature to handle form posts too. It's a relatively straightforward extension of what was there before.

The great majority of this PR concerns E2E tests, in part because I reorganized the enhanced nav tests into a subfolder and changed their URLs to make things more organized now we are getting so many E2E tests for SSR.

@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Jun 21, 2023
@SteveSandersonMS SteveSandersonMS marked this pull request as ready for review June 21, 2023 11:40
@SteveSandersonMS SteveSandersonMS requested a review from a team as a code owner June 21, 2023 11:40
Comment on lines -1 to -26
@using Components.TestServer.RazorComponents.Pages.Forms
@using Components.TestServer.RazorComponents.Shared

<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="utf-8" />
<base href="/subdir/" />
<HeadOutlet />
</head>
<body>
<Router AppAssembly="@typeof(FormWithDefaultContextApp).Assembly">
<Found Context="routeData">
<RouteView RouteData="@routeData" DefaultLayout="@typeof(FormsLayout)" />
<FocusOnNavigate RouteData="@routeData" Selector="h1" />
</Found>
<NotFound>
<PageTitle>Not found</PageTitle>
<LayoutView>
<p role="alert">Sorry, there's nothing at this address.</p>
</LayoutView>
</NotFound>
</Router>
<script src="_framework/blazor.web.js" suppress-error="BL9992"></script>
</body>
</html>
Copy link
Member

Choose a reason for hiding this comment

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

Is this not used?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not a big deal, but this was only used to vary the layout, so I added a _Imports.razor to set the layout for forms test pages instead. It's only so we can keep the E2E tests runnable manually (by launching TestServer), which is much easier when they have a common root component.

For cases where it's completely necessary to vary the root component I guess we have no choice, and then would have to add some new entry to the list of test apps, but whenever we can avoid growing that it will be easier to find the test case you want.

@SteveSandersonMS SteveSandersonMS merged commit 0b632fa into main Jun 21, 2023
@SteveSandersonMS SteveSandersonMS deleted the stevesa/progressively-enhanced-form-post branch June 21, 2023 16:51
@ghost ghost added this to the 8.0-preview6 milestone Jun 21, 2023
@javiercn
Copy link
Member

I don't know if there are tests for redirects, specifically, should we add them?

@SteveSandersonMS
Copy link
Member Author

It will work with PRG already because the existing enhanced nav logic handles redirects pretty thoroughly.

What won't be supported here is a 307 redirect (it will behave like a 302 instead). I'll file an issue to track that. What's your view on the level of importance of supporting 307?

@SteveSandersonMS
Copy link
Member Author

SteveSandersonMS commented Jun 21, 2023

Actually that's only partly true. I think (but haven't confirmed) it will work with a 307 as long as it's a true 307 from the server and that it goes to a URL in the same origin, based on the expectation that fetch will follow such redirections and give you the result.

What wouldn't work is if you're in the middle of streaming rendering and try to issue a 307 - you simply can't because there's no way to set a status code. Our existing mechanism handles NavigationException by sending a command to the JS code that handles it like a 302. I haven't put in any API that would allow the developer to communicate an intent to do a 307.

So for now I don't think there's any action to take on that, but if we get developer feedback that they need to be able to describe other types of redirections during streaming SSR, we'd have to take that as a future enhancement.

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.

2 participants