Codecov Report

Merging #4639 (97ce0e9) into dev (68a80de) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev    #4639      +/-   ##
==========================================
- Coverage   91.39%   91.37%   -0.02%     
==========================================
  Files         366      365       -1     
  Lines       12573    12535      -38     
==========================================
- Hits        11491    11454      -37     
+ Misses       1082     1081       -1     

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 68a80de...97ce0e9. Read the comment docs.

Thanks for your work on this.

I did quite a bit of work on this previously but couldn't get it to work.
Are you sure turning off the trim analyser doesn't affect functionality?
How are you dealing with all the reflection?
What happens when you use one icon. Does it add the whole class as a dependency?

What happens when you use one icon. Does it add the whole class as a dependency?

If we use some dissembler and look inside the trimmed library, we won't even see Icons there.
If the project uses an Icon, it will copy the value to its project, I think the image is self-explanatory

this is taken from my project. The reference icon turned into the string in my project meanwhile there is no presence of Icons.Rounded.Phone in the MudBlazor.dll

How are you dealing with all the reflection?

I didn't see much reflection in the MudBlazor except the MudBlazor.Docs for the Icons for example and one place that uses Expression for filtering?

Are you sure turning off the trim analyses doesn't affect functionality?

By default it's disabled, I explicitly set the property because it makes sense to enable it in the future.
Enabling it would fail the complication because 4 components are not trim friendly, in total there are 10 warnings.
MudFormComponent(validation only)
MudDataGrid
FilterDefinition(which only uses MudDataGrid)
EventManager

This are the problematic methods:
ValidationContext(Form validation)
Expression.Property(only MudDataGrid)
JsonSerializer.Serialize(mainly MudDataGrid)
JsonSerializer.Deserialize(mainly MudDataGrid)

So my bet is that mostly only the MudDataGrid grid is broken, which I understand will be reworked anyway.

So there is chance it won't work properly with trimming(but not necessarily). We need to investigate it.
For now as separate PR we can mark those components/methods(some are public) as RequiresUnreferencedCode to make it clearer to the developer that this are not trim friendly. Later we can rewrite some to make it trim friendly. Nothing too criminal.

Let me first say I appreciate this PR 👍
Bugs that turn up due to trimming are very hard to diagnose so I just want to be sure you have fully assessed the reasons and affects of ignoring the trim warnings.

Are you sure turning off the trim analyses doesn't affect functionality?

By default it's disabled,

I am not sure that is true. Remove it and recompile and the build will fail

For now as separate PR we can mark those methods as RequiresUnreferencedCode

That leads you down a path that I failed on as once you mark one method you have to mark the dependencies and so on and so on.

Probably the only way is to make the relevant methods trim friendly.

Its up to the core team if we want to ignore the trim warnings @henon @just-the-benno

I am not sure that is true. Remove it and recompile and the build will fail

You are correct, for some reason I thought that by default it's disabled.

Bugs that turn up due to trimming are very hard to diagnose so I just want to be sure you have fully assessed the reasons and affects of ignoring the trim warnings.

Ok, tmr I will make analysis and check on the MudBlazor.Docs with trimming to see what got broken and how bad it is. My project obviously doesn't use all the components so I don't know how usable it is actually.
You have a point, I guess it's not best idea to leave the developer with bugs that might happen because of trimming and maybe then we should remove IsTrimmable so they would use it only at their own risk, on the other hand trimming is not officially yet supported in .net6 for aspnetcore which doesn't stop people from using it and we have to start from some point.

This is great work would be a shame to lose momentum on it. I hope to have a look at the trim warnings as well. Maybe there are some easy wins.

@ScarletKuro Thank you very much for making a non-breaking PR. And yes, next major version we'll remove the old way of using the icons in favor of the new structure if @Garderoben agrees.

Wow! Amazing! and @henon yes, yes yes!

I uploaded it as a dev nuget 6.0.11-dev.1
https://www.nuget.org/packages/MudBlazor/6.0.11-dev.1

I guess your PR will break it, won't it?

Yes, this PR will break it. But looking at the script it shouldn't be hard to adopt it.

Ok, tmr I will make analysis and check on the MudBlazor.Docs with trimming to see what got broken and how bad it is. My project obviously doesn't use all the components so I don't know how usable it is actually.

Did this today. I tried trimming with MudBlazor.Docs to see what will work and what not. The MudBlazor library was something like ~1.2mb.
Mainly because for the Icons reflection was used and they got trimmed, and we will not count this as "broken" for obvious reason.

Components that got broken for sure:
MudSelect
MudScrollToTop

I didn't really find yet why MudSelect got broken, but when you click on the Item, the dropdown doesn't disappear.
MudScrollToTop is easy to explain, because:

[JSInvokable]
public void RaiseOnScroll(ScrollEventArgs e)
{
     _onScroll?.Invoke(this, e);
}

It's using JSInvokable and has no references, so it's simply got trimmed. There are other places with such issue, for example: KeyInterceptor, ResizeListenerService, ScrollSpy maybe some more but it's not hard to find those. My bet is that if we mark them with DynamicallyAccessedMembersAttribute \ DynamicDependency we are good to go. (will make a PR for this later)

The rest actually looks like working fine. I had no issues with form validation, didn't see bugs in the MudDataGrid either.

Okay, I think i fixed all interop, and now MudSelect and MudScrollToTop is working.
@henon now I have a question, should I update this PR with interop trim fixes or make new PR?
Do I need to comment in code why I marked those classes with attributes or just explanations in the ticket will be enough?

@JonBunator updated powershell script.

@JonBunator updated powershell script.

Thank you!

@ScarletKuro Great work. I would suggest adding the fixes to this PR. We don't want to merge known bugs even if the next PR fixes them. @henon @Garderoben after these fixes and before we merge should we publish the PR to MudBlazorTest ?Trimming bugs don't hit the test suite. Maybe the core team could run through the docs to see if they can find any broken bits and then compare to dev.

Added to PR. the pattern was taken from aspnetcore.
I also had to mark not only some methods but types too, because they were used by JS but not by the library, like ScrollEventArgs. Or for example KeyOptions and KeyInterceptorOptions was used by lib but only the setters, and getters would be trimmed, when getters are needed for interop serlization.

I think I can fix some TrimAnalyzer warnings, the JsonSerializer with SourceGenerator, but this will be somekind of breaking change.
This one for example

    [JsonSerializable(typeof(EventArgs))]
    [JsonSerializable(typeof(ChangeEventArgs))]
    [JsonSerializable(typeof(ClipboardEventArgs))]
    [JsonSerializable(typeof(DragEventArgs))]
    [JsonSerializable(typeof(ErrorEventArgs))]
    [JsonSerializable(typeof(FocusEventArgs))]
    [JsonSerializable(typeof(KeyboardEventArgs))]
    [JsonSerializable(typeof(MouseEventArgs))]
    [JsonSerializable(typeof(PointerEventArgs))]
    [JsonSerializable(typeof(ProgressEventArgs))]
    [JsonSerializable(typeof(TouchEventArgs))]
    [JsonSerializable(typeof(WheelEventArgs))]
    internal sealed partial class WebEventJsonContext : JsonSerializerContext
    {
    }

And then it will be trimm friendly. The breaking thing here is that it won't accept any kind of type.

I made a new nuget with the new changes for testing: https://www.nuget.org/packages/MudBlazor/6.0.11-dev.2

@Garderoben @henon Is it ok to use MudBlazor-Test to publish this with docs? I saw it was used recently.

Sure, why not?

Sure, why not?

Just wanted to make sure someone wasnt using it for a current piece of work because I saw it was at 6.0.10

I published this to

https://mudblazor-test.azurewebsites.net/

As discussed no icons in there as the reflection is not annotated but at least this tells us its trimmed version of docs.

https://mudblazor-test.azurewebsites.net/features/icons#icons

See if we can find any more issues.

@mikes-gh did you host the MudBlazor.Docs.Wasm not the MudBlazor.Docs.WasmHost? If yes then as side note the table/datagrid won't be able to load the api(webapi/periodictable / webapi/americanStates) to show anything. Also if u want you can edit/remove the Content Security Policy in index.html to get rid of some errors in console.

Youre right I'll republish

OK Done sorry for delay. vscode publish wasnt working so had to use my work machine to publish with vs 2022

@ScarletKuro Could you fix the material icons in the docs please. I see for custom icons the class is instantiated so the custom icons don't get trimmed but material still only uses reflection so they are not in MudBlazor.dll. Of course this means that the docs wont have much saving from trimming but this is expected as the docs showcase nearly all the features. Or we could turn off trimming for the docs but there might be some savings? (I added <PubishTrimmed>true</PublishTrimmed> to MudBlazor.Docs.Wasm). Or maybe we could lazy load the full set of icons if they were in a separate library.

@Garderoben For the purposes of the docs how about serving the icon page svg strings from the wasm host via api. That way we can keep the larger MudBlazor.dll server side and use only the smaller version in the WASM startup.

OK Done

Ty, now it's working

Could you fix the material icons in the docs please

Ok i will look this one when I will have time.

Or we could turn off trimming for the docs

If you just turn off the PubishTrimmed for wasm client then the libs won't be trimmed

I have another question. Does it make sense to fix MudDataGrid trimm warnings or not? Because as I understood that someone is working on DataGrid anyway?

We can avoid those either by using expression overload with type which is already marked with DynamicallyAccessedMembers

public static MemberExpression Property(Expression? expression [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties|DynamicallyAccessedMemberTypes.NonPublicProperties)] Type type, string propertyName)

else if(isNumber){
    var nullableDoubleType = typeof(double?);
    var isnotnull = Expression.IsTrue(Expression.Property(field, nullableDoubleType, "HasValue"));
    var isnull = Expression.IsFalse(Expression.Property(field, nullableDoubleType, "HasValue"));
}
else if (isBoolean){
    var isnotnull = Expression.IsTrue(Expression.Property(field, typeof(bool?), "HasValue"));
}
else if (isDateTime){
    var nullableDateTimeType = typeof(DateTime?);
    var isnotnull = Expression.IsTrue(Expression.Property(field, nullableDateTimeType, "HasValue"));
    var isnull = Expression.IsFalse(Expression.Property(field, nullableDateTimeType, "HasValue"));
}

or using wrapper

internal class NullableProperty<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] T> where T : struct
    public static PropertyInfo HasValue { get; } = typeof(T?).GetProperty("HasValue");
}

else if(isNumber){
    var isnotnull = Expression.IsTrue(Expression.Property(field, NullableProperty<double>.HasValue));
    var isnull = Expression.IsFalse(Expression.Property(field, NullableProperty<double>.HasValue));
}
else if (isBoolean){
    var isnotnull = Expression.IsTrue(Expression.Property(field, NullableProperty<bool>.HasValue));
}
else if (isDateTime){
    var isnotnull = Expression.IsTrue(Expression.Property(field, NullableProperty<DateTime>.HasValue));
    var isnull = Expression.IsFalse(Expression.Property(field, NullableProperty<DateTime>.HasValue));
}

it is already released as a preview nuget https://www.nuget.org/packages/MudBlazor/6.0.11-dev.3
but @Garderoben will very soon release v6.0.11

@henon I'm rather waiting for version v6.0.11 for the fixes concerning memory leaks. I hope it will be available soon

@Garderoben I ask again because I think it's important to have a new official (non-dev) version available to fix memory leaks. When will the release be available?

There have been several inquiries about the release date of v6.0.11 but unfortunately no response as of yet.
I even tried asking on Discord to no avail.

According to NuGet stats, v6.0.10-dev4 has been downloaded 2500 times already and browsing through the MudBlazor Issues,
it seems there are no specific issues related to this dev4 release. In other words: It seems (very) good to go.

In our case, we are eagerly awaiting to make some new features available to our customers
using components that depend on the MudDropZone component; v6.0.10-dev4 has some crucial fixes to make it
work as expected.

Somehow I can't help to feel that MudBlazor has lost traction over the last weeks.
Many, many issues still have triage status and the list of PR's doesn't seem to get shorter.

Please don't get me wrong, I do realize this is an open-source project and I very much appreciate
all the hard work that everybody is contributing to MudBlazor to make it even better.
If there are other things going on, then please let us know, post a message or something, but don't keep your
users in the dark.

My team already asked me twice now if I'm still sure that we made the right decision to choose MudBlazor.
I'm still a 100% confident about my decision, but in the back of my mind a countdown timer has started.

@ScarletKuro I personally use a fork of the dev branch on my project to get the latest features and I can also fix bugs and open PRs at the same time. I asked myself the same things, should I or not go with MudBlazor because of the overhead. Many form components are bugged and should be rewritten from scratch but still, this is an excellent open source project. A paid version or paid support could probably help.

I think @Garderoben wanted to do the release two weeks ago but then something got in the way. That is exactly the reason why the preview releases exist, so we can make the fixes available to you all without much work on our part (no need to write the changelog and all the other adminstrative stuff @Garderoben does when he releases). He hasn't been feeling so well the last weeks so please give him some space and time.

And yes, it is not easy to be there and serve the community all the time. Especially with that volume of issues and PRs we have. We'd really have to do this full time to be able to keep up. Until the time is right to go that way with paid support etc, we kindly ask you guys to sort out issues yourself and fix bugs you need fixed for your projects. It still requires a lot of work on our part to review the PRs. For any PRs which are time critical bug fixes you can request a review from me. I'll try to review as soon as I have a free hour and I'll do preview releases regularly to get those fixes out to the community.

@Garderoben I ask again because I think it's important to have a new official (non-dev) version available to fix memory leaks. When will the release be available?

@boukenka There would not be much different between an official release and a dev preview except the name and the version. There is no guarantee that an "official" release contains less bugs than a dev preview.

@ScarletKuro thanks!

FYI v6.0.11 has finally been released.

Unfortunately I have a breaking change after upgrading. But only because I have a special requirement in one project, I use reflection to get the icon from the Icons.Filled class. Is there a way to prevent this trimming for a specific project?

@MarianSWA yes, there is a way. You can look commits of this PR, I did this for the docs.
You need a storage with DynamicallyAccessedMembers attribute on type and use it like this.
Also, pay attention that its now const properties, you need .GetFields with different BindingFlags to get that.

.GetFields(BindingFlags.Public | BindingFlags.Static | BindingFlags.FlattenHierarchy);

And to get the value its GetRawConstantValue call instead of .GetValue.

Another, more advance and harder way is to use LinkerConfig.xml and add this type to exception.

Let me know if you need additional help with that.

@ScarletKuro The first solution worked perfectly, thanks for the detailed answer 😄