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

[wasm] Implement System.Console.Clear #43002

Merged
merged 4 commits into from
Oct 8, 2020

Conversation

campersau
Copy link
Contributor

In #41184 (comment) it was mentioned that System.Console.Clear should be implemented since now it is not marked as UnsupportedOSPlatformAttribute("browser") but throws PlatformNotSupportedException.

public static void Clear() => throw new PlatformNotSupportedException();

So this PR does that by using the same code pattern as in BrowserHttpClient:

private static readonly JSObject? s_fetch = (JSObject)System.Runtime.InteropServices.JavaScript.Runtime.GetGlobalObject("fetch");

<ItemGroup Condition=" '$(TargetsBrowser)' == 'true'">
<ProjectReference Include="$(LibrariesProjectRoot)System.Private.Runtime.InteropServices.JavaScript\src\System.Private.Runtime.InteropServices.JavaScript.csproj" />
</ItemGroup>

Fixes mono/mono#19825

@ghost
Copy link

ghost commented Oct 3, 2020

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

@@ -74,6 +76,8 @@ public override void Flush()

internal static class ConsolePal
{
private static readonly JSObject? s_console = (JSObject)System.Runtime.InteropServices.JavaScript.Runtime.GetGlobalObject("console");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since Console.Clear might not be used that much, I can move the initialization into Clear itself if needed.

Copy link
Member

Choose a reason for hiding this comment

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

I think I'd prefer it happen in clear for now with a try catch that wraps the missing object exception just so we don't suddenly start throwing here if console is missing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my testing System.Runtime.InteropServices.JavaScript.Runtime.GetGlobalObject did not throw an exception if the object does not exist, and just returns null.

try
{
    var abc = System.Runtime.InteropServices.JavaScript.Runtime.GetGlobalObject("abc");
    if (abc is null) Console.WriteLine("abc is null");
    else Console.WriteLine($"abc is {abc.GetType().FullName}");
}
catch (Exception ex)
{
    Console.WriteLine($"{ex.GetType().FullName}: {ex.Message}\n{ex.StackTrace}");
}

logged

abc is null

Calling Invoke("clear") would throw if the method does not exists:

System.Runtime.InteropServices.JavaScript.JSException: Error: Method: 'abc' not found for: '[object Object]'

Should this be catched and ignored?

@ghost
Copy link

ghost commented Oct 4, 2020

Tagging subscribers to this area: @eiriktsarpalis, @jeffhandley
See info in area-owners.md if you want to be subscribed.

@marek-safar marek-safar added the arch-wasm WebAssembly architecture label Oct 4, 2020
@marek-safar marek-safar requested a review from lewing October 4, 2020 07:37
Copy link
Member

@lewing lewing left a comment

Choose a reason for hiding this comment

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

@campersau thanks again for another catch. We're considering some more extensive changes for console in 6 but this doesn't need to wait for that. I'm curious if this has any size impact.

@lewing lewing requested a review from tqiu8 October 5, 2020 02:16
@lewing
Copy link
Member

lewing commented Oct 6, 2020

The failures here appear to be from #43037

@lewing
Copy link
Member

lewing commented Oct 8, 2020

Failures are #43166 and unrelated to the change

@lewing lewing merged commit c533b60 into dotnet:master Oct 8, 2020
@campersau campersau deleted the browserconsoleclear branch October 8, 2020 15:36
lewing pushed a commit to lewing/runtime that referenced this pull request Oct 9, 2020
* [wasm] Implement System.Console.Clear

* Add dummy console.clear implementation if it does not exist

* Initialize console in Clear

* Apply suggestions from codereview
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-System.Console
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[WASM] Console.Clear() not calling console.clear in devtools
5 participants