Skip to content
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

Fix warning for @formname without @onsubmit #9296

Merged
merged 2 commits into from
Sep 19, 2023

Conversation

jjonescz
Copy link
Member

@jjonescz jjonescz commented Sep 18, 2023

Related to #9077.
Discovered when updating SDK in ASP.NET Core repo: dotnet/aspnetcore#50734

This removes warning RZ10021: Attribute '@formname' can only be used when '@onsubmit' event handler is also present. which was reported in wrong cases (even if user had valid @onsubmit) plus it became clear that it's not needed at all (@formname should be allowed without @onsubmit).

(The warning condition was wrong because the integration tests didn't register/process the event like real ASP.NET Core setting would - I fixed that in this PR for the @formname tests by adding @using Microsoft.AspNetCore.Components.Web.)

@jjonescz jjonescz added the area-compiler Umbrella for all compiler issues label Sep 18, 2023
@jjonescz jjonescz marked this pull request as ready for review September 18, 2023 13:47
@jjonescz jjonescz requested a review from a team as a code owner September 18, 2023 13:47
333fred
333fred previously approved these changes Sep 18, 2023
@SteveSandersonMS
Copy link
Member

Could you clarify why there even is a warning/error for Attribute '@formname' can only be used when '@onsubmit' event handler is also present? When would it occur?

If interpreted with no other context, it's not true - you can have either @formname or @onsubmit without the other. They are both useful on their own, so we shouldn't be blocking that.

In all cases, @formname should compile as a call to AddNamedEvent and not as a call to AddAttribute. If it did compile as AddAttribute, then it would not work at runtime as soon as we finish removing the temporary API support.

@jjonescz
Copy link
Member Author

you can have either @formname or @onsubmit without the other

Ah, OK, I thought it's not allowed, probably looking at the runtime implementation:

https://github.com/dotnet/aspnetcore/blob/d3122c7d16b85812891aced9c23e975b46f58f12/src/Components/Endpoints/src/Rendering/EndpointHtmlRenderer.EventDispatch.cs#L117

I will remove the warning completely then.

In all cases, @formname should compile as a call to AddNamedEvent and not as a call to AddAttribute. If it did compile as AddAttribute, then it would not work at runtime as soon as we finish removing the temporary API support.

Even if allowing @formname without @onsubmit, there's still one case where it won't compile to AddNamedEvent - when used on a non-<form> element (as there's also a warning for that). That is fine?

@SteveSandersonMS
Copy link
Member

Ah, OK, I thought it's not allowed, probably looking at the runtime implementation

Understood. There is a misleading comment left behind there. Fortunately we do have E2E test cases that exercise @formname without @onsubmit.

Even if allowing @formname without @onsubmit, there's still one case where it won't compile to AddNamedEvent - when used on a non-

element (as there's also a warning for that). That is fine?

Yes, that's fine. Thanks for checking! There is currently no use case for @formname on elements other than <form>, so the warning and non-support is good. If there's ever a reason to change that we'll coordinate with you.

@jjonescz jjonescz requested a review from 333fred September 19, 2023 08:59
@jjonescz jjonescz dismissed 333fred’s stale review September 19, 2023 09:05

Removed the warning completely

{
return RazorDiagnostic.Create(FormName_MissingOnSubmit, source ?? SourceSpan.Undefined);
}
// Removed warning RZ10021: Attribute '@formname' can only be used when '@onsubmit' event handler is also present.
Copy link
Member

Choose a reason for hiding this comment

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

Think we can delet this comment

// Act
var generated = CompileToCSharp("""
<form method="post" @onsubmit="() => { }" @formname="named-form-handler"></form>
<form method="post" @onsubmit="() => { }" @formname="@("named-form-handler")"></form>
Copy link
Member

Choose a reason for hiding this comment

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

For this test it appears no code is really generated and no diagnostics are emitted. Is that correct? If so it seems a bit odd to me that we do nothing and don't alert the user.

Copy link
Member Author

@jjonescz jjonescz Sep 19, 2023

Choose a reason for hiding this comment

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

The @formname is translated into AddNamedEvent normally - https://github.com/dotnet/razor/blob/f808c223fdbdbc2a05069005c9693022d4cc693e/src/Compiler/Microsoft.AspNetCore.Razor.Language/test/TestFiles/IntegrationTests/ComponentRuntimeCodeGenerationTest/FormName_MissingUsing/TestComponent.codegen.cs. The @onsubmit is emitted as-is but that's a pre-existing behavior - all events behave like this when the event handler is not registered.

Could perhaps implement a warning for this specific scenario - like "you seem to be using @formname with broken @onsubmit, did you mean to have event handler for @onsubmit registered?" - but that's something that shouldn't happen to users normally and if we want to do that, I guess it could be a follow up PR.

@333fred 333fred merged commit 8848273 into dotnet:main Sep 19, 2023
@ghost ghost added this to the Next milestone Sep 19, 2023
@jjonescz jjonescz deleted the 9077-FormName-ExtraWarning branch September 20, 2023 07:32
@Cosifne Cosifne modified the milestones: Next, 17.8 P3 Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-compiler Umbrella for all compiler issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants