Skip to content

Better implementation of EventCallback.Equals #53362

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

Closed
wants to merge 0 commits into from

Conversation

JelleHissink
Copy link
Contributor

Better implementation of EventCallback.Equals

We noticed after upgrading our blazor application to .net 8.0 that some EventCallback keep being modified. Eventhough the value was the same, the same target component, the same method.

The generated code that renders the component seems to fabricate a new MulticastDelegate, having the same Type, Target and Method. The implementation of EventCallback uses Object.ReferenceEquals, so it concludes it is different every render.

Fixes #53361

@JelleHissink JelleHissink requested a review from a team as a code owner January 14, 2024 13:31
@ghost ghost added area-blazor Includes: Blazor, Razor Components community-contribution Indicates that the PR has been added by a community member labels Jan 14, 2024
@ghost
Copy link

ghost commented Jan 14, 2024

Thanks for your PR, @JelleHissink. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@ghost ghost added this to the 8.0.x milestone Jan 14, 2024
@ghost
Copy link

ghost commented Jan 14, 2024

Hi @JelleHissink. If this is not a tell-mode PR, please make sure to follow the instructions laid out in the servicing process document.
Otherwise, please add tell-mode label.

@JelleHissink
Copy link
Contributor Author

@dotnet-policy-service agree

@JelleHissink
Copy link
Contributor Author

I wanted to reproduce the build error locally, however I am having issues running all tests locally because I get this error when I execute restore.cmd:

C:\Work\github\aspnetcore\eng\targets\Npm.Common.targets(46,5): error : [email protected]: The engine "node" i
s incompatible with this module. Expected version "^14 || ^16 || ^17 || ^18 || ^19". Got "20.11.0" [C:\Work\github\aspn
etcore\src\SignalR\clients\ts\common\common.npmproj]

@SteveSandersonMS
Copy link
Member

Would it be OK to retarget this to main so it goes in .NET 9? Targeting release/8.0 is much more complicated as we need a much more definite justification for risking other behavioral changes in a patch release.

Bear in mind that the change being introduced here doesn't only affect EventCallback.Equals; it also affects rendering as it can change which set of components get detected as needing an update according to the diffing system. It doesn't just revert to .NET 7 behavior; it creates a different behavior for rendering than existed in .NET 7 or 8. I agree that the change is a desirable one in the long term, but we try to be extremely careful about introducing subtle behavioral changes in patch releases.

I know you need a solution for your own app before then, but perhaps you could work around by calling your own EventCallback comparison utility function.

@JelleHissink
Copy link
Contributor Author

Sorry, I managed to totally get stuck in changing the target branch, I made a new PR #53395
@SteveSandersonMS Could you assign the same properties there?

@ghost
Copy link

ghost commented Jan 15, 2024

Hi @JelleHissink. 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 community-contribution Indicates that the PR has been added by a community member
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

EventCallback.Equals is not giving correct results
2 participants