Skip to content

Conversation

eerhardt
Copy link
Member

This is the first change necessary. It removes the one place that depends on Console.WriteLine.

This first change doesn't save much size (.br compressed):

  • Before: 2,719,214 bytes
  • After: 2,719,111 bytes

However, if we can also remove the last place that references System.Console, we should be able to trim the whole assembly (6KB .br compressed).

private static void HandleStartupException(Exception exception)
{
// Logs to console, and causes the error UI to appear
Console.Error.WriteLine(exception);
}

Speaking with @lewing, we thought if we unified the startup paths between the mono runtime and Blazor, we should be able to remove this last dependency on System.Console. @lewing was going to log an issue to track unifying the startup code.

cc @marek-safar

@eerhardt eerhardt requested a review from a team as a code owner February 17, 2021 20:24
@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Feb 17, 2021
Copy link
Contributor

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

I approve as a non-stakeholder.

@javiercn
Copy link
Member

However, if we can also remove the last place that references System.Console, we should be able to trim the whole assembly

I think it could be doable, we would need to call some JS interop API instead to notify JS of the error. @SteveSandersonMS do you have any thoughts/concerns?

@eerhardt
Copy link
Member Author

@captainsafia - any thoughts on this PR? I think it is ready to merge, unless you have other feedback.

@javiercn
Copy link
Member

@eerhardt can we let @SteveSandersonMS comment on the last System.Console usage to see if we can just update this PR and trim the entire assembly?

@eerhardt
Copy link
Member Author

if we can just update this PR and trim the entire assembly?

My understanding is to trim the entire System.Console assembly is going to take a bit of refactoring how we initialize a Blazor WASM app, and it may take coordination with @lewing's team.

@javiercn
Copy link
Member

My understanding is to trim the entire System.Console assembly is going to take a bit of refactoring how we initialize a Blazor WASM app, and it may take coordination with @lewing's team.

Ah, thanks for the clarification.

Then let's get this merged!

Thanks for the PR

@javiercn
Copy link
Member

@eerhardt do you want to merge the PR yourself or are you ok if we do it for you

@SteveSandersonMS
Copy link
Member

Speaking with @lewing, we thought if we unified the startup paths between the mono runtime and Blazor, we should be able to remove this last dependency on System.Console. @lewing was going to log an issue to track unifying the startup code.

I'm sure we can remove the Console.Error.WriteLine call from HandleStartupException independently of any unification to the startup paths. Since that call is coming from the .WebAssembly assembly, it could directly call WebAssembly.JSInterop.InternalCalls.InvokeJS to get the log message into JS world.

Or if that's inconvenient, we could even have a dedicated InternalCall method specifically for logging errors, which would bypass all dependencies on any JS interop (and maybe there already is one).

Copy link
Member

@captainsafia captainsafia left a comment

Choose a reason for hiding this comment

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

Looks good from my end!

We can take Steve's recommendation on cleaning up the System.Console use in the entrypoint invoker and finish this off.

@eerhardt eerhardt merged commit 7bbaec1 into dotnet:main Mar 1, 2021
@eerhardt eerhardt deleted the RemoveConsole branch March 1, 2021 14:59
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants