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.

Sign up for GitHub

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

Jump to bottom

JSInterop: Enable returning null for a nullable value type #60850

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

JSInterop: Enable returning null for a nullable value type #60850

wants to merge 3 commits into from

Conversation

Regenhardt
Copy link
Contributor

@Regenhardt Regenhardt commented Mar 10, 2025 •
edited
Loading

JSInterop: Enable returning null for a nullable value type

  • 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.
  • 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

@Regenhardt
JSInterop: Enable returning null for a nullable value type
8b02460 
When a result value is null, and the return type is a nullable value type, Convert.ChangeType throws instead of returning the null value.

dotnet#30366
@Regenhardt Regenhardt requested a review from a team as a code owner March 10, 2025 16:03
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-blazor Includes: Blazor, Razor Components label Mar 10, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Mar 10, 2025
@Regenhardt
Copy link
Contributor Author

Pushed a draft of what I wanted the tests to look like, gotta figure out the test fixture stuff too.

@Regenhardt Regenhardt force-pushed the feature/null-value-type-from-js-runtime branch from 6f7d836 to 2dad3d3 Compare March 10, 2025 22:08
@Regenhardt
Copy link
Contributor Author

Regenhardt commented Mar 10, 2025 •
edited
Loading

Ok I kinda figured out how the tests are supposed to work. Just have to install jdk now tomorrow to test locally.

@Regenhardt Regenhardt mentioned this pull request Mar 12, 2025
Closed
@Regenhardt Regenhardt force-pushed the feature/null-value-type-from-js-runtime branch from 2dad3d3 to fe68842 Compare March 12, 2025 22:32
@Regenhardt Regenhardt force-pushed the feature/null-value-type-from-js-runtime branch from fe68842 to 0a86864 Compare March 13, 2025 15:33
@Regenhardt
Copy link
Contributor Author

Figured it out, tests are green now.

@Regenhardt
Copy link
Contributor Author

Alternative: use result == default instead of result == null && default(T) == null as it might be slightly faster?

@Regenhardt Regenhardt mentioned this pull request Mar 13, 2025
Closed
4 tasks
@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

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.

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Mar 21, 2025
@Regenhardt
Copy link
Contributor Author

Ready for review.

pavelsavara
pavelsavara reviewed Mar 21, 2025
View reviewed changes
src/JSInterop/Microsoft.JSInterop/src/Infrastructure/TaskGenericsUtil.cs
@@ -89,7 +89,9 @@ public void SetResult(object tcs, object? result)
// If necessary, attempt a cast
var typedResult = result is T resultT
? resultT
: (T)Convert.ChangeType(result, typeof(T), CultureInfo.InvariantCulture)!;
: result == null && default(T) == null // ChangeType can't convert null to value types
Copy link
Member

Choose a reason for hiding this comment

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

It seems OK to me.

@oroztocil please review

@build-analysis build-analysis bot mentioned this pull request Mar 21, 2025
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pavelsavara pavelsavara

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
area-blazor Includes: Blazor, Razor Components community-contribution Indicates that the PR has been added by a community member pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

jsRuntime.InvokeAsync can't return null as Nullable<Guid>
2 participants
@Regenhardt @pavelsavara