Named events tracking

[SteveSandersonMS]

This is polish for the named events tracking.

It integrates it fully into the rendering-and-diffing system, so information about named events is held in the rendertree (as a new RenderTreeFrame type), and additions/removals/changes are all detected by the existing diffing system. The payoff is that we don't need custom storage for this information or to hook mutate renderer state during rendering, plus edge cases that were difficult before now work naturally (e.g., component re-renders and changes its @onsubmit delegate).

More general alternative

At one point I generalized this to a concept of "named values", i.e., arbitrary name-value pairs that can exist at locations in the rendertree, and we track when they are added/removed/moved/changed. It's very easy to build "named events" on top of this as a special case.

However I couldn't really justify adding a completely general feature like that when we have only one use case for it, so I degeneralized it back to named events. I'm pointing this out because maybe in the future we would have some reason to round-trip stable-named information across multiple SSR requests. If that arises, we can very easily replace the NamedEvent frame type with NamedValue and re-generalize it.

[SteveSandersonMS]

Unrelated, but I realised the ComponentRenderMode frames were not yet handled by RenderBatchWriter. It just needs to disregard them. This helper supports the unit test for that.

[javiercn]

It integrates it fully into the rendering-and-diffing system, so information about named events is held in the rendertree (as a new RenderTreeFrame type), and additions/removals/changes are all detected by the existing diffing system. The payoff is that we don't need custom storage for this information or to hook mutate renderer state during rendering, plus edge cases that were difficult before now work naturally (e.g., component re-renders and changes its @onsubmit delegate).

This is good, I think at the time, the reason why I did not do it this way was because we were trying to minimize the perf impact on non-server scenarios. This change obviously comes at a cost, but I think it's worth it, and if you are happy with it, so am I.

At one point I generalized this to a concept of "named values", i.e., arbitrary name-value pairs that can exist at locations in the rendertree, and we track when they are added/removed/moved/changed. It's very easy to build "named events" on top of this as a special case.

This is something that might have helped the diffing algorithm too on preserving the DOM nodes, as it could act in a similar way as to how @key works.

Overall, the changes look great, it's much cleaner when integrated on the Render Tree frames, as opposed to us having to do all the tracking internally, so I think its worth the extra public API.

There are a couple of areas that I think we need to re-think a bit:

• Dealing with ambiguity.
• Dealing with non-registered form handlers.

Dealing with non-registered form handlers

We probably want to make this a 404.

• It's not an error, you can receive a POST from external sources. (We can still include info in the 404 page during development indicating what's going on).
  • Razor pages would for example, return a 404 if you don't have an OnPost method in your page/model.

Dealing with ambiguity.

• I don't think we should preclude the page from rendering just because it generated ambiguous event handlers.
• The nature of the event handling system is very dynamic, and that means that if you make a mistake, it's not crazy that you might deploy that to production without catching it.
  • The previous behavior would still allow the page to render, and for information to be displayed.
  • The outcome is that an error would prevent the user from dispatching the form but would still let him see the data.
• With the current behavior, an error in defining a unique form name will result in your site being unusable, even for users that are only viewing pages.
  • I think that is a worse user experience, as it makes the site unusable, as opposed to limiting it to the form POSTing aspect of it.

As an example, in ASP.NET Core link generation when there is an ambiguity in the link generation process, the page continues to render. In older versions it returned an empty string, in the current version it uses the first route that matches. It does this because a broken/incorrect link is much better than the page not being useable at all.

With form posting it should be the same. If the developer made a mistake and was not caught during testing, we should minify the scope of the error as much as possible. There's no value in us preventing the page from rendering in production, it doesn't help the developer, and it hurts the user experience.

If we want to make this obvious, we should log a message when this happens (a warning/error) so that it can show up in the logs and addressed, or if we want to be a bit more proactive, throw during development. However, there's no point on throwing in production for this during rendering, it just hurts the user experience and makes a development mistake much worse.

[SteveSandersonMS]

We probably want to make this a 404.

It would be nice, but unfortunately I didn't see a good way to do that. The previous logic gave a 404 if there are no submit event handlers on the page, or a 500 if there is a submit event handler but it's not the one you submitted. I don't think that distinction makes sense, as it would cause a page to change from being a 404 to a 500 on post if you add a completely unrelated form elsewhere on it.

In general we should assume there may be unrelated forms on the page (e.g., a login form in a nav bar or layout).

We could potentially do a 404 if a POST specifies a handler and we can't find it, or if it is a POST request and doesn't specify a handler. Would you prefer that, or do you consider that more confusing during development?

I don't think we should preclude the page from rendering just because it generated ambiguous event handlers.

This was a deliberate change. I think it is much better to fail fast, because otherwise it's really hard to know if a problem will occur. Consider rendering a list of items, each of which makes a form. If there might be duplicates in the list, you'd never realise there was a problem until you try to submit one of the duplicated items (whereas the non-duplicated items could be submitted). Blocking it up front is much clearer.

The nature of the event handling system is very dynamic, and that means that if you make a mistake, it's not crazy that you might deploy that to production without catching it.

Deploying semi-functional sites to production is not a desirable outcome :) I'd err on the side of giving people the best chance to catch it. Having semi-functional sites where there are forms that cause errors on submission is not a benefit.

In the unlikely event that we get feedback that people genuinely want to have these semi-broken experiences, we will be free to loosen the restriction in the future. But if we ship it like this initially, we'll never be able to tighten it and give developers a stronger guarantee that their site works. Would you be OK with starting with the more rigorous checks and loosening it in the future if it becomes clear that people need that for some reason?

[javiercn]

We probably want to make this a 404.

It would be nice, but unfortunately I didn't see a good way to do that. The previous logic gave a 404 if there are no submit event handlers on the page, or a 500 if there is a submit event handler but it's not the one you submitted. I don't think that distinction makes sense, as it would cause a page to change from being a 404 to a 500 on post if you add a completely unrelated form elsewhere on it.

In general we should assume there may be unrelated forms on the page (e.g., a login form in a nav bar or layout).

We could potentially do a 404 if a POST specifies a handler and we can't find it, or if it is a POST request and doesn't specify a handler. Would you prefer that, or do you consider that more confusing during development?

Would it work to make it a 404 in all cases? I don't think it should be a 500.

This was a deliberate change. I think it is much better to fail fast, because otherwise it's really hard to know if a problem will occur. Consider rendering a list of items, each of which makes a form. If there might be duplicates in the list, you'd never realise there was a problem until you try to submit one of the duplicated items (whereas the non-duplicated items could be submitted). Blocking it up front is much clearer.

The nature of the event handling system is very dynamic, and that means that if you make a mistake, it's not crazy that you might deploy that to production without catching it.

Deploying semi-functional sites to production is not a desirable outcome :) I'd err on the side of giving people the best chance to catch it. Having semi-functional sites where there are forms that cause errors on submission is not a benefit.

The opposite is also true, if this goes past the development stage, you end up with an unusable site in prod, magnifying the consequences, as opposed to an error on the specific functionality that has a bug in it.

There are trade-off on both sides, could we struck a middle ground where we throw in development and simply Log in production and only fail in the event of an actual post? This type of thing can mean that if you render a widget on the layout dynamically, your app might be completely broken, and it might not happen in your testing.

[javiercn]

Is there a reason why we are removing this check? The goal here was to ensure that only one component defines a specific named event between the render starts and we go to dispatch the form.

This is meant to avoid circumstances where two components pick the same name and just happen to render at different points in the cycle.

[SteveSandersonMS]

I removed it because I thought this previous restriction was just a limitation in the old implementation. The new implementation always knows the complete set of named events and always immediatley untracks any that were removed.

As such we no longer need to block the form from binding in this case. As long as, by the time we reach quiescence, only one named event is registered with a given name, that's the one to invoke. It's desirable that people should be free to update their component hierarchy arbitrarily. Just because some other form previously existed and used a certain name shouldn't stop components added later from using that name (as long as the old one is now gone).

[SteveSandersonMS]

just happen to render at different points in the cycle.

I think what we care about is the state of the component hierarchy at the point of quiescence. There shouldn't be any random timing issues with this, as quiescence is a deterministic thing.

[javiercn]

Signing off so you can merge, but can we at least file issues for the error experience on ambiguous forms? I would be more at ease if we only break hard during development.

If you are unconvinced by it, could you at least file an issue so that we can discuss it within the team? It truly feels that we are maximizing the consequences for a bug in production for no added benefit.

[SteveSandersonMS]

Would it work to make it a 404 in all cases? I don't think it should be a 500.

I agree a 500 is wrong. But do you think it would be reasonable to return a 400? I think that makes the most sense overall, since it really is a bad request. Someone's done a POST to an endpoint that does exist but won't accept method=POST unless the request also specifies a valid handler, so the request itself is invalid.

Either way I'll merge this now so I can keep moving forwards but will follow up on changing this status code.

There are trade-off on both sides, could we struck a middle ground where we throw in development and simply Log in production and only fail in the event of an actual post? This type of thing can mean that if you render a widget on the layout dynamically, your app might be completely broken, and it might not happen in your testing.

I agree, it's a tradeoff, and your points are totally reasonable. On balance I'm OK with doing this as you've suggested and failing only at event dispatch time. I'll file an issue to follow up on that.

[SteveSandersonMS]

Filed #49359 and #49362