vikramlearning / blazorbootstrap

An Enterprise-class Blazor Bootstrap Component library built on the Blazor and Bootstrap CSS frameworks.
https://docs.blazorbootstrap.com/
Apache License 2.0
565 stars 15 forks source link

Coding Rework #739

Closed Lucasharskamp closed 2 weeks ago

Lucasharskamp commented 4 weeks ago

This PR is dependent on the completion of https://github.com/vikramlearning/blazorbootstrap/pull/737

Before we start: this PR isn't as dramatic as you might think it is

Barely any features have been altered of the library. What has been changed is for the sake of readability, nullability operators where needed, redundant checks being removed, typos fixed and bad language practices rectified. Performance also increased.

Here are a list of all the changes:

Added support for .NET 7

The library now also builds to .NET 7.

XML Documentation

All <summary> documentation now gets exported when making a nuget package, ensuring all users get to have it, which is great for Github CoPilot and user usage. It also means you now have 3000 build warnings for lacking documentation, which I hope will be resolved in due time

(almost) all incoming data is now of type IReadOnlyCollection<T>, not IEnumerable<T>

The goal of the library is to present information to the front-end and relay events back for handling. The task of the library is not to handle data. IEnumerable<T>'s problem is that it way too often involves a contract that gets executed when the collection gets read, leading to multiple iteration issues. IReadOnlyCollection<T?> doesn't have this issue, unless you're trying very, very hard. On the opposite end, List<T> restricts the programmer too much. The blazor bootstrap library only needs a collapsed collection of items to iterate over for display/filter/sorting, but doesn't alter the collection in any way. So any collapsed collection will do. And unlike List, IReadOnlyCollection<T?> allows one to enter any collection type they want; an array T[], List<T>, ImmutableList<T>, etc.

It isn't the library's business to decide what kind of data is entered, as long as it is in a valid state

For more information, see: https://stackoverflow.com/a/51484883 https://learn.microsoft.com/en-us/dotnet/api/system.collections.generic.ireadonlycollection-1 (see the derived list)

Added tons of comments

This isn't very exhaustive yet, but for the components and some methods I've added XML documentation. Within the documentation, I've also added references to langwords (null, true, false, etc) and references to other objects. All components have a XML summary.

Pass-through Task's through the callstack

Methods declared as async and using await are converted into a state machine by the compiler. However, in many cases, we're only propagating a Task up the callstack. In that case, the method doesn't need to await a task and turn itself into a state machine, but simply propagate the Task upwards for the parent to deal with. This adds that the 'correct' Task gets returned, e.g. a ValueTask in some cases.

Redundant checks on collection modification removed

This isn't Framework; you don't need to check if a collection contains X before removing X.

Added nullability references to objects

Some objects weren't marked as being allowed to be null, even though the code anticipates that eventuality. In those cases, I've added a ? to the property

Mark methods as static if the contents aren't altered

This prevents one from needing to create and maintain objects if we're never working with its contents, but only methods that are static anyways. (Looking at you, sidebar 1 and sidebar 2)

Proper variable/method naming conventions

No lowercase method/property names, s.v.p. For JS invokable's, just provide an attribute parameter. Also, no properties that only exist to propagate a parsed dataset to frontend. That also causes multiple iteration issues!

Fixed improper exception throwing

If parameter x is null, you throw an ArgumentNullException. If x.Foo is null, you must throw an ArgumentException. Fixed this issue on several places.

Update to example websites

All example websites now use IReadOnlyCollection implementations and use the sidebar handlers properly.

Prevent unnecessary StateHasChanged() invocation

When updating properties, StateHasChanged needs to be invoked for .NET6 but not .NET7/8. This is rectified using Macro's.

Expected changes for users

Not all that much; the only real thing that changes for users is that the data they're submitting can't be of type IEnumerable anymore, meaning the user needs to collapse the stack before adding it. Will probably fix a few bugs users unwittingly caused. Other than a bit of performance improvement or getting better help from CoPilot, no changes should happen to user's experiences or output.

Lucasharskamp commented 2 weeks ago

Closed because of merging issues introduced after this PR was made. Making new one with extra features.