JSInterop: Enable returning null for a nullable value type

[Regenhardt]

JSInterop: Enable returning null for a nullable value type

• [x] You've read the Contributor Guide and Code of Conduct.
• [ ] You've included unit or integration tests for your change, where applicable.
• [ ] You've included inline docs for your change, where applicable.
• [x] There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Summary of the changes (Less than 80 chars)

Description

When a result value is null, and the return type is a nullable value type, Convert.ChangeType throws instead of returning the null value, so in case off null this now uses null directly.

Fixes #30366

[ghost]

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

[Regenhardt]

Not entirely sure how to react if the value is null but T is not nullable so I figured keep the existing reaction of Convert.ChangeType.

[Regenhardt]

Or I could also rebase onto master and force push :-)

[Regenhardt]

Can someone explain this CI error to me? I don't understand what's wrong:

dotnet\sdk\9.0.100-alpha.1.23618.1\NuGet.targets(169,5): error : (NETCORE_ENGINEERING_TELEMETRY=Build) Cannot create a file when that file already exists.

[halter73]

The CI error appears to be infra flakiness that has cleared up.

Not entirely sure how to react if the value is null but T is not nullable so I figured keep the existing reaction of Convert.ChangeType.

This seems reasonable to me. I'm guessing you still get an InvalidCastException which isn't great, but no worse than before as you said.

Can you add a regression test for this fix? It doesn't look like there's a ton of testing in this area, but I think you could put something in BasicTestApp and write an E2E test using Selenium along the lines of StartupErrorNotificationTest but in a new test class. Other people on the @dotnet/aspnet-blazor-eng might have better suggestions, but that's where I'd probably start.

[dotnet-policy-service[bot]]

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime. To make sure no conflicting changes have occurred, please rerun validation before merging. You can do this by leaving an /azp run comment here (requires commit rights), or by simply closing and reopening.

[mkArtakMSFT]

Can you add a regression test for this fix? It doesn't look like there's a ton of testing in this area, but I think you could put something in BasicTestApp and write an E2E test using Selenium along the lines of StartupErrorNotificationTest but in a new test class. Other people on the @dotnet/aspnet-blazor-eng might have better suggestions, but that's where I'd probably start.

@Regenhardt do you plan to address the feedback here? We try to maintain high quality bar and improve it in areas where it lacks. Without additional tests we won't be able to proceed with this change.

[Regenhardt]

Yes I'm definitely planning on putting this under test, I just need some time for I'm currently writing my bachelor's thesis so that has priority.

[dotnet-policy-service[bot]]

Hi @Regenhardt. It seems you haven't touched this PR for the last two weeks. To avoid accumulating old PRs, we're marking it as stale. As a result, it will be closed if no further activity occurs within 4 days of this comment. You can learn more about our Issue Management Policies here.