Do not trimm methods marked with JSInvokable

[ScarletKuro]

This is just an offer, but while creating this doc issue dotnet/AspNetCore.Docs#28630
I was wondering whatever it would make sense to just not trim those methods by default.
More detailed explanation in the issue above, but
TL;DR: In case of JS Interop (Call .NET from JS) the linker will remove methods with JSInvokable because usually there will be no references to them.
While inspecting aspnetcore source code I found a way how to deal with it.
But due to lack of documentation, many people do not know that their method might be trimmed and how to solve it. My offer is either to document it somewhere or make it so that the linker will not touch methods marked with JSInvokable.

[vitek-karas]

@pavelsavara - who might know what the intention here is.

[pavelsavara]

The overall problem is that such code, when only used by JS is not statically visible as being used by linker.

For [JSInvokable] which is blazor only construct, documenting the [DynamicDependency] is probably way to go.

This is similar but not same as problem with [JSExport] attribute.
Since JSExport triggers code generator it generates code to keep itself alive.

[vitek-karas]

This is similar but not same as problem with [JSExport] attribute.

Just curious - can [JSExport] be used instead of [JSInvokable] since among other things it solves this problem?

[pavelsavara]

Just curious - can [JSExport] be used instead of [JSInvokable] since among other things it solves this problem?

Probably yes, but I don't know all use cases nor done gap analysis.

@danroth27 is there desire for this ?

[danroth27]

Hmm...I'm not sure. We don't know statically if these methods are being used or not. What's the linker philosophy on situations like this? Do we err on the side of aggressively trimming things and requiring the user to tell us if we shouldn't? Or do we err on making things work at the expensive of not trimming stuff that may not be used? If someone is writing a library that uses these attributes would our guidance be that they always need to be marked so they don't get trimmed? If that's our guidance, then it would be nice if the attributes themselves could signal that to the trimmer without additional metadata getting manually added by the user.

[pavelsavara]

That's interesting point about libraries. [JSExport] is always protecting the code from trimming.
We could have way how to opt-out in the future. I imagine flag on the attribute.

[ScarletKuro]

[JSExport] is always protecting the code from trimming.

Interesting that here https://github.com/dotnet/aspnetcore/blob/main/src/Components/WebAssembly/WebAssembly/src/Services/DefaultWebAssemblyJSRuntime.cs the DynamicDependency is still used together with JSExport. I assume it's because no one did a cleanup (or the lack of documentation that JSExport prevents from trimming due to source generator).

[ScarletKuro]

FYI: the ongoing PR documentation change dotnet/AspNetCore.Docs#28631

[pavelsavara]

@MackinnonBuck we could cleanup DynamicDependency on JSExport. But perhaps there will be further refactoring of those helpers.

[vitek-karas]

From a trimmer perspective it's "trim as much as possible", but that's just the tool.

I think the right question is what is the SDK's take and more specifically what is our designed goal for a given vertical.

For example in console apps we're more strict and there we would require the code to declare these dependencies such that trimmer can see them (either via DynamicDependency, generated code, or some XML descriptors).

On the other hand for Blazor, or Xamarin verticals we tend to "just make things work", which would likely lead to automatically rooting these. That said, the default for Blazor/Xamarin is to NOT trim user code or NuGets, so this should not be a problem there - and if a given library opts into aggressive trimming, it would need to root these somehow explicitely.

[ScarletKuro]

Could depend on TrimMode. full - trim as much as possible, partial - make things work. This would make sense, I only recently found out that WASM doesn't even support full mode (from docs I thought its the default?), the razor components / pages get removed #3160 (comment)

[danroth27]

for Blazor, or Xamarin verticals we tend to "just make things work", which would likely lead to automatically rooting these.

Makes sense

if a given library opts into aggressive trimming, it would need to root these somehow explicitely

It's common to use JSInvokable in a library to provide reusable JS interop implementations and I think we would want to encourage these library authors to make their libraries trimmable. Currently I assume that means the library author would need to manually annotate JSInvokable methods as DynamicDependency. It would be nice if there was a way for JSInvokable to indicate that it always represents a DynamicDependency without additional user action. Is that already possible today?

[ScarletKuro]

Another idea: Maybe it would be possible to add a warning when EnableTrimAnalyzer is enabled and JSInvokable is missing DynamicDependency.

Currently I assume that means the library author would need to manually annotate JSInvokable methods as DynamicDependency.

Yes.

The blazor doc is now updated regarding JSInvokable and it should be easier for library authors to support trimming.
Should I close the ticket? Tbh, I only noticed now that this repo is archived.

[vitek-karas]

Just to add what other discussions similar to this ended up with:

The primary problem with adding functionality like "Always keep all methods with the JSInvokable attribute" is that it's unconditional. That goes against the main ideas of trimming - only leave actually used code. Such an unconditional statement basically means - these methods are always going to be used. There are also problems defining exactly what it means (what if the app has an assembly which is not used by anything, but it contains JSInvokable methods - should it still keep those and thus the assembly?). It's actually rarely the case that the methods should be kept always - for example if I add a reference to a new library (and not use it yet), trimming should be able to remove that assembly.

So in general we will try to figure out if there's some other mechanism which could make it less unconditional. The JSExport attribute along with the source generator does a bit better job as it makes the methods conditionally kept on the assembly itself being kept. It's possible that we can't do better than that, but it's not something the trimmer should know about - it should be told.

For future issues/questions, please create them in the dotnet/runtime repo - since the trimmer code base moved there.