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

[HybridWebView] Always serialize the value in JS when invoking #27068

Merged
merged 15 commits into from
Jan 17, 2025

Conversation

mattleibow
Copy link
Member

@mattleibow mattleibow commented Jan 10, 2025

Description of Change

Previously, there was a special case for calling back into .NET from JavaScript that would not convert a string to JSON. This made sense, a string is already a string, so no need to convert to a JSON string.

But...

This would mean if the response was undefined or null, then the JS would serialize to "undefined" or "null". Then we would see that the .NET expected a string and say well, return it directly. This would mean that a null in JS would end up a "null" - string value.

We can't add a special logic in .NET to convert "null" into a null, because what would happen if the JS function wanted returned a "null" string? It would then be converted to null.

This PR solves all the issues by removing the special case of string and always serializes to JSON. This means that if the JS function returned the string "null", it would get serialized into a string "\"null\"" (with quotes); but a null string would be serialized into a string "null" (no quotes). The deserializer in .NET would then basically remove a layer of quotes giving use the correct result: "\"null\"" becomes a string again, but "null" becomes null.

Issues Fixed

Fixes #26765

This change just makes adding new tests easier
@Copilot Copilot bot review requested due to automatic review settings January 10, 2025 16:19
@mattleibow mattleibow requested a review from a team as a code owner January 10, 2025 16:19
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 4 changed files in this pull request and generated 1 comment.

Files not reviewed (2)
  • src/Controls/tests/DeviceTests/Resources/Raw/HybridTestRoot/index.html: Language not supported
  • src/Controls/tests/DeviceTests/Resources/Raw/HybridTestRoot/invokedotnettests.html: Language not supported

@mattleibow mattleibow added this to the .NET 9 SR3 milestone Jan 10, 2025
@mattleibow mattleibow changed the base branch from main to dev/hybridwebview-tests January 10, 2025 19:48
…return

# Conflicts:
#	src/Controls/tests/DeviceTests/Elements/HybridWebView/HybridWebViewTests.cs
@mattleibow mattleibow marked this pull request as draft January 10, 2025 19:53
@mattleibow mattleibow changed the base branch from dev/hybridwebview-tests to main January 10, 2025 20:10
@mattleibow mattleibow marked this pull request as ready for review January 13, 2025 11:45
@mattleibow mattleibow modified the milestones: .NET 9 SR3, .NET 9 SR4 Jan 13, 2025
@mattleibow mattleibow added the area-controls-hybridwebview HybridWebView control label Jan 13, 2025
@mattleibow mattleibow requested a review from Eilon January 13, 2025 13:41
@mattleibow mattleibow self-assigned this Jan 13, 2025
@mattleibow mattleibow removed their assignment Jan 15, 2025
@mattleibow mattleibow changed the title Don't deserialize a string from JS into a .NET string, return it directly [HybridWebView] Don't deserialize a string from JS into a .NET string, return it directly Jan 15, 2025
Eilon
Eilon previously approved these changes Jan 15, 2025
@mattleibow mattleibow changed the title [HybridWebView] Don't deserialize a string from JS into a .NET string, return it directly [HybridWebView] Always serialize the value from JS when invoking Jan 17, 2025
@mattleibow mattleibow changed the title [HybridWebView] Always serialize the value from JS when invoking [HybridWebView] Always serialize the value in JS when invoking Jan 17, 2025
Copy link
Member

@Eilon Eilon left a comment

Choose a reason for hiding this comment

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

Easy!

@mattleibow mattleibow merged commit 4a36584 into main Jan 17, 2025
100 of 104 checks passed
@mattleibow mattleibow deleted the dev/hybridwebview-string-return branch January 17, 2025 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-controls-hybridwebview HybridWebView control
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

HybridWebView doesn't accept strings returned by JS method invoke
2 participants