rdeago
commented
2 years ago @geoperez of course I appreciate your thumbs-up (also because it's been the only feedback so far 🤦♂️) but I'm having a bit of a hard time figuring it out.
Do you mean you approve of all proposed changes? If so, shall I take it as your personal approvation, or also on behalf of Unosquare? (Does Unosquare still use EmbedIO, by the way?)
Also, which version of SWAN do you think is best as a dependency? Frankly, the situation with SWAN has become confusing, what with version 4.0.0 plus various packets at version 6.0.x in different beta test stages. SWAN's repo is not of much help either, with README still referencing version 3.
geoperez
gabriele-ricci-kyklos
maarlo
Sorry for all-caps-yelling in the title.
The release of .NET 6 consolidated a number of changes in the .NET ecosystem that we cannot ignore for much longer.
I've thus started working on a new major release of EmbedIO. I mean, newer than the current
masterbranch. I will keep track of the work in progress in this issue.There are some changes I have not done yet, because they could negatively impact some users. I need comments. I need to know whether I can go forward with some changes, or postpone them.
Basic guidelines
Enforce code style
A consistent coding style is important, especially in projects with (hopefully) several contributors. With the release of Visual Studio 2022 (
.editorconfigsupport, lots of styling hints) and the integration of code analyzers into the .NET project system, we have no more excuses.I have enabled both standard .NET analyzers and StyleCop analyzers in EmbedIO. After a fair amount of trial-and-error cycles, we now have a combination of
.editorconfig,StyleCop.Analyzers.ruleset, andstylecop.jsonfiles that make analyzers not stumble into one another.Most informational analyzer messages have been elevated to the rank of warnings. In addition, when building in Release mode, warnings (including code style warnings) are now treated as errors. This is to ensure that no PR can be successfully merged unless it follows code style rules.
More analyzers may be added in the future, e.g. Public API analyzers.
Use modern C# language features
I have already addressed the massive amount of compiler warnings related to non-nullable reference types. Lots of them required subtle, but breaking, changes to public APIs.
I'm striving to eliminate the use of
nullcompletely, especially in public-facing APIs, defining "null objects" (such asWebServer.NullUri) where necessary as substitutes for null references. This is an ongoing effort that started with version 3.0.Similarity to HttpListener APIs is not a requirement
HttpListener APIs suffer from both historical technical debt and the need to be compatible with Windows'
http.sysdriver's APIs.Wherever there's a better or simpler way to use HttpListener than the one represented by its APIs, EmbedIO should embrace the simpler way, with "bridge" classes (
SystemHttpContextet al.) translating it into HttpListener API calls. We have interfaces practically for everything for a reason, after all.A example of the above is the refactoring of WebSocket support, although API simplification was more of a byproduct than the main reason behind it.
Simplify classes applying the Single Responsibility Principle
Some EmbedIO classes are more complex than necessary (ranging from "slightly more complex" to "nearly unmaintainable") because they try to be many things at once.
Take
WebServerBase, for example:Its "service" and "main loop" natures overlap quite a lot in code, making it hard to e.g. modify lifetime management without messing up the use of HttpListener.
Instead, we need a base class that is only a service, doing something in a background loop, with all the start / stop / fatal error management logic but no knowledge that HttpListener even exists. From it we can derive our
WebServerBase, with the added bonus that we can derive other types of services too if we want, all with the same lifetime management APIs.This was only an example, of course (I'm almost done implementing it, by the way). The general principle is that, in the words of Robert C. Martin, _"a class should have only one reason to change"_; in other words, when you look at the code for
WebServerBase, you should only see HttpListener initialization, use, and disposal - not service lifetime management, not MIME type customization.The fact that
WebServerBasealso implementsIMimeTypeCustomizeris, to tell the truth, a design flaw, and it's all the fault of yours truly. I should have implemented a dependency injection mechanism with hyerarchical scopes instead of cramming functionality downWebServerBase's (andModuleGroup's, andFileModule's) throat. It can be remedied though. Read on. :wink:Use more abstractions
We need a layer of abstraction over logging, so one can keep using SWAN or use Serilog, NLog, or whatever logging library one desires.
We need a layer of abstraction over serialization, so one can keep using SWAN or use Newtonsoft.Json, System.Text.Json, etc.
(We need a layer of abstraction over HttpListener... no, wait, we've had that from day one. Yay for @mariodivece!)
We need a layer of abstraction over any feature (especially if orthogonal to the main concern of a web server, which is to handle requests and produce responses) that can be provided by a third-party library, with a minimum-viable-functionality default to internal or SWAN classes.
Incoming changes
OK, enough theory! :smile: Here's a list of the changes coming in next PR:
.editorconfig.async-friendly implementation of lifecycle methods (issue with details coming soon)WebServerBasederive from the generic service classFeel free to add to this list, especially if there is some open issue you want to see fixed soon.
Proposed changes
I really need comments on what follows.
Implement middlewares as a separate class
Some existing modules (think
IPBanningModule, Extras'BearerTokenModule, etc.) are really middlewares. They are different from "normal" modules because:Given that their runtime semantics are different, they should be a separate class, with two methods in place of
HandleRequestAsync: one called before passing the request to modules, the other after a module has produced a response. (Actually, it's a bit more complicated than this, but you get the idea.)Once middleware mechanics are in place, I will remove the
IHttpContext.OnClosemethod, which is a hack I added to avoid implementing middlewares in version 3. :grimacing:Rethink
IsFinalHandlerI'd like to find a way to remove
IWebModule.IsFinalHandler; at a minimum, it needs to be specified per-instance when needed.Limit
WebApiModuleto a single controllerOnce we can specify that a
WebApiModuleis not a final handler (see previous paragraph), there is no need for it to support more than one controller.Need two controllers on the same base route? Just use two modules, of which the first has
IsFinalHandlerset tofalseso it passes unmatched requests forward instead of responding with404 NotFound.Split the library
EDIT: general-purpose utility types already in separate library
EmbedIO.Utilities, see #550."No additional dependences" has been a basic tenet of EmbedIO development since @mariodivece wrote v1.0. The only exception is SWAN, which keeps us from reinventing the wheel.
However, things have changed since 2013. .NET applications can now be published ready-to-run and trimmed, thus reducing the burden associated with possibly unused dependencies.
(Moreover, even if EmbedIO had some additional dependences, it could hardly beat the obscene amount of DLLs that any ASP.NET Core application has to carry around. :see_no_evil:)
Some parts of
EmbedIO.dllcould be split into their own libraries, with advantages that depend upon the specific code being moved:IHttpContext) into their own library would enforce separation of concerns and prepare EmbedIO for the announced death of Microsoft's HttpListener;The disadvantages of a split library would amount to a few, if any, more dependencies to specify in application projects. (Again, we can hardly beat ASP.NET on that.)
Use dependency injection
Support for dependency injection has been asked for before, especially by users coming from ASP.NET.
Some EmbedIO features can be refactored to take advantage of dependency injection. Notable examples are:
We can either depend upon
Microsoft.Extensions.DependencyInjection.Abstractions, or roll our own abstraction (for which I already have a half-baked spec in mind - more on that soon, in a separate issue).We will also need a default implementation, so as not to force everyone to choose a separate dependency injection library even for small, proof-of concept applications.
Support more logging engines
EDIT: Once SWAN is gone (see #548) we're most probably going to use
Microsoft.Extensions.Logging. See also #475.Some of us use NLog. Some (including myself) use Serilog. Some even have their own logging library.
As soon as you have some peculiar requirement, like e.g. logging to a cloud service, SWAN just doesn't cut it.
We can either depend upon
Microsoft.Extensions.Logging.Abstractions, or roll our own.The default implementation will probably be logging to the console via SWAN.
Support more serialization libraries
I envision a group of libraries (and NuGet packets) under the
EmbedIO.Serializationumbrella, each containing the same types (e.g. the "famous"JsonDataAttribute), with the same public interfaces (save for some library-specific customization methods) but under different namespaces:EmbedIO.Serialization.Swan,EmbedIO.Serialization.Newtonsoft.Json,EmbedIO.Serialization.System.Text.Json, etc.Adding support for more serialization libraries (e.g.
EmbedIO.Serialization.System.Xml) would be trivial. They would also be used all the same way, so switching all Web API controllers from SWAN to Json.NET would entail just a dependency name change and a find & replace operation.This could require some work to write abstractions for serialization features; then the various
EmbedIO.Serialization.*libraries would essentially be "bridges" towards the actual serialization libraries.The only disadvantage would be that, from v4.0 on, to use serialization at all one should add one or more dependencies, chosen among the available
EmbedIO.Serialization.*packets.Move EmbedIO.Extras to this repository
Moving EmbedIO.Extras projects to this repository would make them a lot easier to keep in sync with EmbedIO itself.
They would even share the same version number with EmbedIO, which would ease application dependency management.
Pinging the usual suspects, in no particular order (feel free to tag others, let's go viral :smile:):
@mariodivece @geoperez @k3zo @madnik7 @rocketraman @bufferUnderrun @maarlo @GF-Huang @AbeniMatteo @gabriele-ricci-kyklos @tiziano-morgia @ghiboz @SaricVr @miquik