Decouple Modular system from Asp.Net MVC #530

Jetski5822 · 2017-01-16T22:15:47Z

ASP.Net Modules

Modules provides a mechinism to have a self contained modular system where you can opt in to a specific application framework and not have the design of you application be dictated to by such.

This directly allows you to decouple the Hosting application from the independent modules. So no more redeploying the whole application for one simple change, instead you can just deploy a single module and recycle you application.

Getting started

First, create a brand new web application.

Within this new application we are initially going to focus on two files, project.json and Startup.cs. If you dont have etiher of these... start again!

Okay so first lets open up project.json.

Within the ConfigureServices method add these lines

services.AddModuleServices(configure => configure
    .AddConfiguration(Configuration)
);

Next, at the end of the Configure method, add these lines

app.UseModules();

Thats it. Erm, wait, what? Okay so right now you must be thinking, well what the hell does this do? good question.

AddModuleServices will add the container middleware to you application pipeline, this means in short that It will look in to a folder called Modules within you application for all folders that contain the mainfest file Module.txt. IF you looked on the file system it would look like this:

MyNewWebApplicaion
  \ Modules
    \ Module1
    \ Module2

Once it has found that manifest file, and said file is valid, it will then look for all classes that implement IStartup, instansiate them and then call the methods on here. An example of one is using the abstract class off of IStartup

public class Startup : StartupBase
{
    public override void ConfigureServices(IServiceCollection services)
    {
        services.AddScoped<ISomeInterface, SomeImplementedClass>();
    }
}

By doing this you allow your modules to be self contained, completely decoupled from the Hosting applicaiton.

Note: If you drop a new module in, then you will need to restart the application for it to be found.

Add Extra Locations

By default module discovery is linked to the Modules folder. This however can be extended.

Within the Startup.cs file in your host, within the ConfigureServices method, add

services.AddExtensionLocation("SomeOtherFolderToLookIn");

Add Extra Manifest filenames

By default the module manifest file is Module.txt. This however can be extended.

Within the Startup.cs file in your host, within the ConfigureServices method, add

services.AddManifestDefinition("ManifestMe.txt", "module");

Additional framework

You can add your favourite application framework to the pipeline, easily. The below implementations are designed to work side by side, so if you want Asp.Net Mvc and Nancy within your pipeline, just add both.

The modular framework wrappers below are designed to work directly with the modular application framework, so avoid just adding the raw framework and expect it to just work.

Asp.Net Mvc

Within your hosting application add a reference to Microsoft.AspNetCore.Mvc.Modules

Next, within Startup.cs modify the method AddModuleServices to look like this

services.AddModuleServices(configure => configure
    .AddMvcModules(services.BuildServiceProvider())
    .AddConfiguration(Configuration)
);

Note the addition of .AddMvcModules(services.BuildServiceProvider())

Thats it, done. Asp.Net Mvc is now part of your pipeline

NancyFx

Within your hosting application add a reference to Microsoft.AspNetCore.Nancy.Modules

Next, within Startup.cs modify the method Configure to look like this

app.UseModules(modules => modules
    .UseNancyModules()
);

Note the addition of .UseNancyModules()

Thats it, done. NancyFx is now part of your pipeline. What this means is that Nancy modules will be automatically discovered.

Note. There is no need to register a Nancy Module within its own Startup class.

Change Notes for Orchard Team

I have moved asp.net mvc out of the Host project Orchard.Cms.Web, and MVC is now a dependency of the Startup project and is also inside of the recipe.

Created a NancyFx implementation, thus allowing us to have Nancy in a module or referenced from the host, similar implementation to MVC.

Removed MVC from the core Orchard startup pipeline

sebastienros · 2017-01-17T17:03:03Z

that inherit off of StartupBase

It should use IStartup instead, or is that a typo?

How did you do to have custom MVC/Razor configuration done from a module?

How is the RouteCollection handed over to a module, or the MVC configuration object?

I will do some local perf tests to ensure the raw perf has not been impacted (theorically it should have no impact) and the multi-tenant scenario is still good (I expect some impact on this one as you told me the MVC pipeline is different now for each pipeline).

Maybe @jersiovic should try it too as the way MVC is setup might impact his branch.

Jetski5822 · 2017-01-17T17:07:21Z

Typo - good catch! - Will answer the other questions on my way home.

sebastienros · 2017-01-17T22:31:11Z

src/Microsoft.AspNetCore.Mvc.Modules/Extensions/ServiceCollectionBuilderExtensions.cs

@@ -67,6 +69,7 @@ public static IMvcCoreBuilder AddExtensionsApplicationParts(this IMvcCoreBuilder
            var availableExtensions = extensionManager.GetExtensions();
            using (logger.BeginScope("Loading extensions"))
            {
+                ConcurrentBag<Assembly> bagOfAssemblies = new ConcurrentBag<Assembly>();
                Parallel.ForEach(availableExtensions, new ParallelOptions { MaxDegreeOfParallelism = 4 }, ae =>


Why parallel here, it's just enumerating a collection.

Not sure in this context but if we still load extensions as we currently do. In the loop LoadExtensionAsync() is called which triggers dyn compilation and / or loading.

Are you saying GetExtensions() is not actually loading them, but only when we enumerate its result?

Yes, GetExtensions() only grab extensions infos based on the search paths options.

jtkech · 2017-01-18T01:13:32Z

src/Orchard.Environment.Extensions/ExtensionLibraryService.cs

+                        _loadedAssemblies
+                            .Values
+                            .Select(x => x.Value.Location));
+            }


I've seen you now pass metadata references to the razor view engine as MetadataReferenceFeature through ApplicationPartManager.FeatureProviders. Wow, did you try it, does it work? If so, cool.

So, what i can say is that normally here we exclude assemblies which are ambient (part of the app). This by using an hashset which is initialized with ApplicationAssemblyNames (see below), not to add them but to exclude them.

jtkech · 2017-01-18T01:34:33Z

src/Orchard.Environment.Extensions/Features/TypeFeatureProvider.cs

@@ -18,7 +21,8 @@ public IFeatureInfo GetFeatureForDependency(Type dependency)
                return feature;
            }

-            throw new InvalidOperationException($"Could not resolve feature for type {dependency.Name}");
+            return CoreFeature;
+            //throw new InvalidOperationException($"Could not resolve feature for type {dependency.Name}");
        }


We removed this since this commit which adds all registered services by modules to ITypeFeatureProvider.

Maybe in this context you need it. Or maybe because you use a service defined in the core but which would need to be registered through a module, as we do for some services through the Orchard.Commons module.

jtkech · 2017-01-18T01:36:08Z

src/Orchard.Environment.Shell.Abstractions/Builders/ShellContext.cs

@@ -27,7 +27,7 @@ public class ShellContext : IDisposable
        /// </summary>
        public IServiceScope CreateServiceScope()
        {
-            return ServiceProvider.GetRequiredService<IServiceScopeFactory>().CreateScope();
+            return ServiceProvider.CreateScope();
        }


This seems to be a new extension method from 1.1 that does exactly what I wrote. So I assume you can remove this method altogether.

Exactly, but maybe we still need the method to use the ServiceProvider of a given ShellContext.

Oh, yes you're right, we could remove it, or keep it as an helper just to write shellContext.CreateScope() in place of shellContext.ServiceProvider.CreateScope().

jtkech · 2017-01-18T02:01:57Z

src/Microsoft.AspNetCore.Mvc.Modules/Mvc/ExtensionMetadataReferenceFeatureProvider.cs

+        {
+            using (var stream = File.OpenRead(path))
+            {
+                var moduleMetadata = ModuleMetadata.CreateFromStream(stream, PEStreamOptions.PrefetchMetadata);


This not to read the entire image into memory 👍

jtkech · 2017-01-18T02:05:00Z

src/Microsoft.AspNetCore.Modules/ServiceCollectionExtensions.cs

+                internalServices.AddOptions();
+                internalServices.AddLocalization();
+                internalServices.AddHostCore();
+                internalServices.AddExtensionManagerHost("app_data", "dependencies");


jtkech · 2017-01-18T04:19:28Z

src/Microsoft.AspNetCore.Mvc.Modules/DesignTimeMvcBuilderConfiguration.cs

-                ((List<MetadataReference>)options.AdditionalCompilationReferences).AddRange(extensionLibraryService.MetadataReferences());
+                var extensionLibraryService = serviceProvider.GetRequiredService<IExtensionLibraryService>();
+                apm.FeatureProviders.Add(
+                    new ExtensionMetadataReferenceFeatureProvider(extensionLibraryService.MetadataPaths.ToArray()));


Anyway here there is a pending PR on DesignTimeMvcBuilderConfiguration to be able to resolve shape tag helpers and where we configure services through orchard ... So, we will see ;)

jtkech · 2017-01-18T04:20:02Z

src/Orchard.Environment.Shell/Builders/ShellContainerFactory.cs

                tenantServiceCollection.Add(startupServices);
+                tenantServiceCollection = new ServiceCollection().Add(tenantServiceCollection.Distinct());


Are you sure? E.g about featureServiceCollection which has been added to featureServiceCollections it seems that here it stays empty. And featureServiceCollections is used later to register feature services in ITypeFeatureProvider.

Then, startupServices.Add(featureServiceCollection) seems to add an empty collection.
Then, maybe you need startupServices.Add(tenantServiceCollection) for other reasons.

jtkech · 2017-01-28T00:36:50Z

@Jetski5822

When i was saying that for MetadataPaths we need to exclude ambient assemblies, maybe it is not necessary because just seen that before adding an assembly in _loadedAssemblies or _compileOnlyAssemblies we always use before IsAmbientAssembly(). So maybe i was wrong, sorry.

That said, as i remember, we need preservecompilationcontext because we use in some places the default DependencyContext. Maybe it could work if you still have Orchard.Cms.Web.deps.json in your bin folder.

Best.

Jetski5822 · 2017-01-28T00:46:39Z

Yo JT - could do with your help on the compilation side of things. I'm trying to stay clear of preservecompilationcontext as it places certain constraints on the environment you roll out to.

…

Sent from my iPhone

On 28 Jan 2017, at 00:36, jtkech ***@***.***> wrote: preservecompilationcontext

jtkech · 2017-01-28T01:13:48Z

No problem i will search on this but i would need more infos on the constraints it places.

jtkech · 2017-01-30T05:36:55Z

@Jetski5822 just done some quick tests but only through Orchard.Cms.Web. Here my findings.

So ok, without preserveCompilationContext there is still a Orchard.Cms.Web.deps.json file (not tried with the cli) and we can still use e.g DependencyContext.Default.RuntimeLibraries. The only difference i've seen is that when the option is used i can get the following.

But without preserveCompilationContext the above collection is empty. We use it to compile modules in the same configuration (Release / Debug) as the ambient app has been built. E.g, the compiler uses it to resolve the binaries paths of referenced projects. Note: When empty we fallback to Default.

That said, if we can't keep this option, here i think we can find another strategy.
Then, i've seen that modules are not compiled / loaded in parallel. I think it's because now there are 1st loaded through LoadFeaturesAsync() when creating the shell. And all LoadExtensionAsync() in a Parallel.ForEach() are now called after. So, maybe just use a parallel loop in LoadFeaturesAsync().

Note: we use MaxDegreeOfParallelism = 4, maybe we can increase it, VStudio uses 8 by default.
Also seen that it creates an app_data folder with a lower case
Then, i started Orchard.Cms.Web and i got the following.
So, i tried a fresh setup, then i got the following.
So, i've tried before your last cleanup commit, then i could run the setup view. But i couldn't submit the setup form. Notice the warning message i had on the 1st view rendering before having input anything.

Best.

Jetski5822 · 2017-01-30T10:10:53Z

@jtkech thanks man for the feedback.

The site loads when I add builder.AddTagHelpersAsServices(); - but you end up with it being in a wierd state.

I've removed the Parallel.ForEach(), and will add it to the extension manager in a later PR when I perf test it.

I have also fixed the app_data casing issue.

If you pull latest you will only see the last error you got, which is to do with missing tag helpers.... I am not sure what is going on to be fair. Even with Compilation Context set to true we would still end up with this issue.

I was trying to avoid preserveCompilationContext as we provide the assemblies at run time. preserveCompilationContext is used for the compilation of razor view, but by providing the assemblies at runtime I have worked around that.

Im just not understanding why the tag helper stuff is being such a pain in the arse at the moment... I am wondering if I am providing something that overrides some other implementation somewhere... I just dont know.

What do you think?

jtkech · 2017-02-01T00:28:24Z

@Jetski5822

preserveCompilationContext

Just re-tried, in fact without this option a .deps.json is still created but with less infos.

From the cli repo: ... .deps.json contains data used for runtime compilation ... without this setting, the compilationOptions will not be present, and it will contain only the runtime targets.

So, no compilation options and only runtime assemblies. Notice that compilation assemblies are not always the same as runtime assemblies. And compile only assembly are not loaded.
We just use the main app compilation options to use the same config (Debug / Release) to compile modules. Then we resolve ourselves modules compilation assemblies through the project model.
But razor compilation also need compilation assemblies retrieved through AssemblyPart which uses DependencyContext, see here.

Test Orchard.Cms.Web

Then, just re-tried with your last commit and, as you, i got the same TagHelper issue. Then, i added AddTagHelpersAsServices() but i still had the weird issue where i can't input anything.
After a quick look, i've seen that now some mvc configurations are done through the Orchard.Mvc module. So it seems that some mvc configurations are done in the context of a child container.
Normally ApplicationParts are already populated with ambient assemblies (our ambient projects, mvc libraries). Then the razor engine retrieve reference from these application parts, see here.
So, i think this is why you now need to retrieve all ambient assemblies. But here, you would need to use DependencyContext (as razor) to retrieve compilation assemblies (not always the same as runtime ones).
Indeed, by moving some configurations to the main app, i could run the setup.

ExtensionLibraryService

If we re-use GetMetadataReferences() (moved to MetadataPaths and not used here), normally we need not to add a _loadedAssemblies if it has its counterpart in _compileOnlyAssemblies. So, maybe better to use Union() in place of Concat().
Note: Here a Lazy initialization was used before because it was called by the razor engine on each request, so to be done only once. But it seems that now this is no more the case.

Best.

Jetski5822 · 2017-02-01T16:40:19Z

@jtkech thanks man, pushed some updates.

I have added a ModularApplicationPart, this should provide all Paths of all assemblies at run time. This is scoped to the request, so that we know we are not serving up modules that are not enabled, which is good.

I have cleaned up the ServiceCollectionExtensions, and added the metadata stuff back in there. That however is no scoped so will need to think about this.

ExtensionLibraryService: reverted the metadata to how you were doing it.

Still hitting that dam TagManager thing. I might have to debug the TagManager stuff.... I think its a bug in their stuff as controllers work....

Jetski5822 · 2017-02-01T17:08:10Z

I think it might be their code... https://github.com/aspnet/Razor/blob/bdbb854bdbde260b3c70f565a93ebbb185a7c5a7/src/Microsoft.AspNetCore.Razor.Runtime/Runtime/TagHelpers/TagHelperTypeResolver.cs#L72

This means that it will try to resolve a reference from the Host and not from the module... Ill override it

sebastienros · 2017-02-02T21:03:52Z

src/Microsoft.AspNetCore.Mvc.Modules/Mvc/ModularApplicationPart.cs

@@ -136,34 +138,12 @@ private static IList<Assembly> GetAssemblies(HashSet<string> assemblyNames, Asse
            return locations;
        }

-        public static IEnumerable<Assembly> GetModularAssemblies(IServiceProvider services)
+        public static IEnumerable<TypeInfo> GetModularAssemblies(IServiceProvider services)


Maybe rename the method?

Jetski5822 · 2017-02-02T21:11:46Z

Totally!

…

Sent from my iPhone

On 2 Feb 2017, at 21:03, Sébastien Ros ***@***.***> wrote: @sebastienros commented on this pull request. In src/Microsoft.AspNetCore.Mvc.Modules/Mvc/ModularApplicationPart.cs: > @@ -136,34 +138,12 @@ private static IList<Assembly> GetAssemblies(HashSet<string> assemblyNames, Asse return locations; } - public static IEnumerable<Assembly> GetModularAssemblies(IServiceProvider services) + public static IEnumerable<TypeInfo> GetModularAssemblies(IServiceProvider services) Maybe rename the method? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

sebastienros · 2017-02-02T23:24:12Z

src/Orchard.Cms.Web/Modules/Orchard.Admin/Startup.cs

-            services.AddScoped<IFilterMetadata, AdminZoneFilter>();
-            services.AddScoped<IFilterMetadata, AdminFilter>();
-            services.AddScoped<IFilterMetadata, AdminMenuFilter>();
+            services.Configure<MvcOptions>((options) =>


agriffard · 2017-02-03T12:37:24Z

src/Microsoft.AspNetCore.Mvc.Modules/LocationExpander/ModulerViewLocationExpanderProvider.cs

 {
-    public class ModuleViewLocationExpanderProvider : IViewLocationExpanderProvider
+    public class ModulerViewLocationExpanderProvider : IViewLocationExpanderProvider


moduler or modular ?

Jetski5822 · 2017-02-03T15:02:26Z

Lots of changes today.

Removed custom exceptions and are now using just the raw Exception class.
Updated nancy module, now its super slick.
created a IModularAssemblyProvider to get all assemblies for a given shell. (name might change)
Lots of renaming and moving things.

Next up..

Remove the Microsoft.AspNetCore.Mvc.Modules.Abstractions project.
move HomePageRoute to its own 'thing'
Introduce a Swagger module

Note: By moving service registration to tenants, we are truly isolating the tenants to their individual sandboxes.

Jetski5822 · 2017-02-03T15:02:44Z

@jtkech check it out and give me your thoughts.

jtkech · 2017-02-03T23:16:18Z

@Jetski5822 Sorry for the delay because of other works, just done some tests and here my findings.

Awesome work, i like the idea of the modular app part which is a types and references provider.

So, because you add mvc services through a module, we need to provide more types and references which are related to the main app, this through app parts or razor options for references.

ModularApplicationPart.Types

Called many times, each call takes 10ms, 20000 types are returned. In fact, here we can 1st consider only extensions but we also need to add dependencies we now load ourselves. So, i've exposed our loaded assemblies and by only doing the following it works, each call takes 1ms and 9000 types are returned.

public IEnumerable<TypeInfo> Types
...
  var serviceProvider = HttpContextAccessor.HttpContext.RequestServices;
  var extensionLibraryServices = serviceProvider.GetRequiredService<IExtensionLibraryService>();
  return extensionLibraryServices.LoadedAssemblies.SelectMany(x => x.DefinedTypes);

ModularApplicationPart.GetReferencePaths()

Called only once and when done here we don't need to do it through razor options. But we can't do it through a regular app part because it would use the dependency context on dynamic loaded modules.

I've seen you use GetReferencedAssemblies() but it is related to loaded runtime assemblies, not compilation ones (which may not be loaded). It can works but you can have e.g the same compilation assembly but different runtime ones according to the environment. By using compilation assemblies (see below) i can see that some of them are referenced in the /refs subfolder where this kind of assemblies are stored.

So, here we can still provide our MetadataPaths but we also need to provide compilation references related to the main app, so to the default dependency context. So, by only doing the following it works. Note: 1st it was not working because i didn't see that preserveCompilationContext on Orchard.Cms.Web was removed again. We need this option to be consistent with runtime compilation, see the last comment here.

public IEnumerable<string> GetReferencePaths()
...
var serviceProvider = HttpContextAccessor.HttpContext.RequestServices;
var extensionLibraryServices = serviceProvider.GetRequiredService<IExtensionLibraryService>();

var assemblyLocations = new List<string>();
assemblyLocations.AddRange(DependencyContext.Default.CompileLibraries.SelectMany(library => library.ResolveReferencePaths()));
var locations = assemblyLocations.Union(extensionLibraryServices.MetadataPaths);

return locations;

Menu

The menu shape implemented through a ShapeTagHelper was not working. It's because services need to be added in the right order, the Orchard.Mvc needs to be called first. I could fix this issue by using a negative priority to this module (higher priorities are called after).

UPDATE: But here modifying the priority breaks the setup, will investigate.

Little things

Unused methods: AddFeatureProvider().
Not sure but in my tests with the above code it also works without AddDefaultFrameworkParts().
If references paths are provided through an app part, no need to do it through razor options.

Dynamic compilation / loading / storing

Here, just to be aware of what we would need because, beyond dynamic compilation, we are very tied to what dynamic loading and storing do specifically.

See all assemblies in Orchard.Mvc bin folder and there is a /refs folder for compilation ones as dotnet build do, and see how this folder is stored in /dependencies. Note: There could be also a /runtimes folder with different runtime ids subfolders as dotnet publish do to store specific runtime assets.

You've done an amazing work and, while we could reconsider what we really need about all the dynamic stuff, you are defining part of the framework dynamically through a module. Just a little afraid :)

Best.

Jetski5822 · 2017-02-04T01:44:26Z

hey @jtkech did you pull latest when you took a look?

jtkech · 2017-02-04T02:29:52Z

No, sorry forgot to mention that i was still on the cleanup and removing things that arent needed commit.

This because i mainly worked on the ModularApplicationPart Types and GetReferencePaths() and i thought that you are using the same here.

So, it works but i'm trying to well understand what happens here and then just add what was missing before. E.g for references, just also add those related to the main app and by using the dependency context to grab compilation assemblies and to be consistent with runtime compilation.

For Types i just re-use the assemblies already loaded when modules was loaded ...

For the menu it's about the main menu on the front end where you have the Home link and which is not displayed. Here we are dependent on the order of module executions. But maybe this now works for you.

Jetski5822 · 2017-02-13T17:57:00Z

Oops, I was running out the door. That doesn't work as MVC doesn't have the required types to compile. Will look when I get back, but yeah... it's a bit dumb

…

Sent from my iPhone

On 13 Feb 2017, at 17:21, Sébastien Ros ***@***.***> wrote: I don't think you get my point, you are still loading all the types of the assemblies. We already talked about it on skype, i thought it was clear. An extension has an Assembly, but can convey multiple features. Each feature can be enabled independently. MVC should only register the types from the enabled features. Hence you can't return all the types of the assembly when for instance a single feature of this extension is enabled. The ShellBluePrint contains a collection of types already filtered with only the types associated to the enabled features. Here is something that should do it: public IEnumerable<TypeInfo> Types { get { return ShellBlueprint .Dependencies .SelectMany(dep => dep.Type); } } As for the other property (GetReferencePath) I don't know what it represent. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

sebastienros · 2017-02-13T19:03:06Z

maybe you need another ApplicationPArt with something else, and it worked because you just returned everything with this one. What are the missing types?

Also you might keep the same current code for GetReferencePaths (this is for RAzor I think)

jtkech · 2017-02-14T01:40:53Z

Here, just for infos the last time i tried nick's branch (not with the latest commits).

Types

Before, by using GetReferencedAssemblies() about 20000 types was provided, but now, because dependency context doesn't work on a dyn loaded assembly, only 259 Types are provided, only those related to main modules assemblies, not their dependencies, which is not enough.

So, i still use GetReferencedAssemblies() on shell features assemblies, but only those which are dyn loaded (app is mvcless) and that reference a primary mvc assembly. Then 1760 Types are provided.

References

Here we need the ambient framework compilation assemblies because AddMvcCore() is not done at the app level. I think we will not need it if we reference modules and then Orchard.Mvc through the main app.

So right now, i use DependencyContext.Default.CompileLibraries.

And, for the non ambient libraries, here also we can't use the dependency context, so right now i use our ExtensionLibraryService.ReferencePaths which uses the project model (here not filtered by features).

Best.

jtkech · 2017-02-14T06:18:55Z

When re-trying my branch, just seen that again the main menu was not always rendered. Not sure it's the case with your branch but i've seen that now the 2 LoadFeaturesAsync() no longer return ordered features.

So, because of the blueprint features order, even Orchard.Mvc has a negative priority, its startup is not always executed first, so shape tag helpers may not be registered ...

Just for infos i could fix it with this temporary workaround.

Best.

Jetski5822 · 2017-02-14T08:18:44Z

@jtkech hang on dude, I'm breaking things at the moment. Use the code before the commit WIP

…

Sent from my iPhone

On 14 Feb 2017, at 06:18, jtkech ***@***.***> wrote: When re-trying my branch, just seen that again the main menu was not always rendered. Not sure it's the case with your branch but i've seen that now the 2 LoadFeaturesAsync() no longer return ordered features. So, because of the blueprint features order, even Orchard.Mvc has a negative priority, its startup is not always executed first, so shape tag helpers may not be registered ... Just for infos i could fix it with this temporary workaround. Best. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

sebastienros · 2017-02-10T20:04:22Z

src/Orchard.Environment.Extensions/ExtensionManager.cs

-            // Results are cached so that there is no mismatch when loading an assembly twice.
-            // Otherwise the same types would not match.
-            return _extensions.GetOrAdd(extensionInfo.Id, (key) =>
+            if (!_isInitialized)


Initialize() is already checking for reentrancy so you don't need it here, just call Initialize() or rename it EnsureInitialized()

sebastienros · 2017-02-10T20:05:36Z

src/Orchard.Environment.Extensions/ExtensionManager.cs

@@ -238,32 +237,43 @@ public IEnumerable<IFeatureInfo> GetDependentFeatures(string featureId)

        public Task<ExtensionEntry> LoadExtensionAsync(IExtensionInfo extensionInfo)


Technically this is not doing a Load, it's doing a Get. A Get should optionally do a Load, and cache it (memoize), and prevent concurrency. The Load might be private and not care about concurrent calls.

sebastienros · 2017-02-10T20:07:55Z

src/Orchard.Environment.Extensions/ExtensionManager.cs

        }

-        public async Task<IEnumerable<FeatureEntry>> LoadFeaturesAsync()
+        public Task<IEnumerable<FeatureEntry>> LoadFeaturesAsync(string[] featureIdsToLoad)


This should be called GetFeaturesAsync().

sebastienros · 2017-02-13T00:48:44Z

src/Microsoft.AspNetCore.Modules/ModularTenantRouterMiddleware.cs

@@ -44,8 +44,7 @@ public class ModularTenantRouterMiddleware
            // Because IIS or another middleware might have already set it, we just append the tenant prefix value.
            if (!string.IsNullOrEmpty(shellSettings.RequestUrlPrefix))
            {
-                string requestPrefix = "/" + shellSettings.RequestUrlPrefix;


What's wrong here?

sebastienros · 2017-02-14T16:02:13Z

src/Microsoft.AspNetCore.Mvc.Modules/SatalliteApplicationPart.cs

+        /// Initalizes a new <see cref="AssemblyPart"/> instance.
+        /// </summary>
+        /// <param name="assembly"></param>
+        public SatalliteApplicationPart(IHttpContextAccessor httpContextAccessor)


Use the ShellBluePrint directly, or your don't know when it will be resolved

I cant, the part gets initialized within a Startup class that doesnt have access to the ShellBlueprint.

sebastienros · 2017-02-14T16:02:27Z

src/Microsoft.AspNetCore.Mvc.Modules/SatalliteApplicationPart.cs

+    /// <summary>
+    /// An <see cref="ApplicationPart"/> backed by an <see cref="Assembly"/>.
+    /// </summary>
+    public class SatalliteApplicationPart :


sebastienros · 2017-02-14T16:03:14Z

src/Microsoft.AspNetCore.Mvc.Modules/SatalliteApplicationPart.cs

+            "Microsoft.AspNetCore.Mvc.ViewFeatures"
+        };
+
+        private bool isInitialized = false;


Private members before constructor

sebastienros · 2017-02-14T16:05:57Z

src/Microsoft.AspNetCore.Mvc.Modules/SatalliteApplicationPart.cs

+            }
+
+            var shellAssemblies = ShellBlueprint.Dependencies.Keys.Select(type => type.GetTypeInfo().Assembly).ToList();
+            var discoveredAssemblies = DefaultAssemblyDiscoveryProvider


Is this code just says that if an assemble has a reference on any of the named assembly from the list, then they are a candidate?

Yes, the code was adapted from the MVC project.

clicked

* Removing dynamic compilation * Removing ModularAssemblyProvider * Work on staticmodular (#580) * Work on staticmodular. * Usings * Moving tag helpers to modules * Caching ShellFeatureApplicationPart ReferencePaths * Resolving only ShellBlueprint types for application parts * Fixing views for precompilation * Sharing ICompilerCache across tenants * Supporting precompiled views * Removing dynamic shape tag helpers * Some cleanups. * Cleanup SharedCompilerCacheProvider. * Formatting

jtkech · 2017-02-18T22:38:56Z

@Jetski5822

Just for infos, after all the updates i also had an issue due to bg tasks running during a fresh setup. If you get the same, see here #583 how i fixed it on my side.

Have a good harvest.

Jetski5822 · 2017-02-18T22:53:10Z

What!!! @jtkech you not gunna be there?

…

Sent from my iPhone

On 18 Feb 2017, at 17:38, jtkech ***@***.***> wrote: @Jetski5822 Just for infos, after all the updates i also had an issue due to bg tasks running during a fresh setup. If you get the same, see here #583 how i fixed it on my side. Have a good harvest. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

jtkech · 2017-02-18T22:54:55Z

No, sorry. Promised, next year ;)

* Fixes building and setup. * Fix the workaround. * A better fix i think for the setup issue.

Jetski5822 · 2017-02-23T15:34:42Z

@sebastienros Can we now merge this?

jtkech · 2017-03-02T05:31:27Z

I'm ok with the current implementation, i was too polarized on assemblies deps and types and then not on those related to shell features. So, here only 2 remarks.

Just for infos mvc uses DefinedTypes and we are using ExportedTypes.
Maybe not important but for infos when we create a tenant container, there are also DIed types that we add to typeFeatureProvider but which are not added to shellBluePrint types.

That said, if it is still possible to have some missing types as we seen with shape helpers, i have another suggestion which maybe will provide a more complete list of types (without custom code).

The strategy is to 1st grab all mvc types by using the mvc DefaultAssemblyPartDiscoveryProvider (not a custom one). Then, we exclude types which are specific to features which are not enabled.

Here, how app mvc types could be grabbed and cached. Then the same list is used by all tenants.

            // we use the public static mvc class which filter mvc types

            _applicationTypes = DefaultAssemblyPartDiscoveryProvider
                .DiscoverAssemblyParts(hostingEnvironment.ApplicationName)
                .Where(p => p is AssemblyPart)
                .SelectMany(p => (p as AssemblyPart).Assembly.ExportedTypes);

            //Note: for each part we could use the regular Types property (but more types)

Then, for a given tenant, something like this.

            var excludedTypes = extensionManager
                .LoadFeaturesAsync().GetAwaiter().GetResult()
                .Except(shellBluePrint.Dependencies.Values.Distinct())
                .SelectMany(f => f.ExportedTypes);

            return GetApplicationTypes()
                .Except(excludedTypes)
                .Select(type => type.GetTypeInfo());

I will do a PR just as a suggestion.

I hope your harvest presentation was great and that there will be a video.

# Conflicts: # src/Orchard.Environment.Shell.Abstractions/Builders/Extensions/ServiceProviderExtensions.cs

Jetski5822 requested review from sebastienros, sfmskywalker, bleroy, DaRosenberg and jtkech January 17, 2017 09:12

sebastienros reviewed Jan 17, 2017

View reviewed changes

jtkech reviewed Jan 18, 2017

View reviewed changes

Jetski5822 force-pushed the mvcless branch from 31cabd2 to 2102703 Compare January 24, 2017 19:12

Jetski5822 force-pushed the mvcless branch from c7e3a52 to 740c1cc Compare January 28, 2017 12:29

sebastienros reviewed Feb 2, 2017

View reviewed changes

agriffard reviewed Feb 3, 2017

View reviewed changes

wip

f9e9d9a

Find dependent assemblies that rely on MVC.

6478095

sebastienros previously approved these changes Feb 14, 2017

View reviewed changes

Jetski5822 and others added 4 commits February 14, 2017 17:37

React to PR comments

c87e231

Fixing build

3ed2c58

Fixing same packages

397bbd1

Jetski5822 and others added 7 commits February 22, 2017 09:58

Fxiign build

0485a92

Fxiign build

bace37d

tweak

b95b952

tweak

aa6f1cb

tweak

f094320

Fixes mvcless building and setup. (#583)

b5ad322

* Fixes building and setup. * Fix the workaround. * A better fix i think for the setup issue.

fixing project

5a65d52

sebastienros added 3 commits March 3, 2017 14:21

Merge branch 'master' into mvcless

b73b80b

# Conflicts: # src/Orchard.Environment.Shell.Abstractions/Builders/Extensions/ServiceProviderExtensions.cs

Rebuilding resources

9416915

Removing duplicated db providers in Setup screen

28e80be

sebastienros merged commit 70c867b into master Mar 3, 2017

Jetski5822 deleted the mvcless branch March 29, 2017 11:14

		tenantServiceCollection.Add(startupServices);
		tenantServiceCollection = new ServiceCollection().Add(tenantServiceCollection.Distinct());

		@@ -238,32 +237,43 @@ public IEnumerable<IFeatureInfo> GetDependentFeatures(string featureId)

		public Task<ExtensionEntry> LoadExtensionAsync(IExtensionInfo extensionInfo)

Decouple Modular system from Asp.Net MVC #530

Decouple Modular system from Asp.Net MVC #530

Conversation

Jetski5822 commented Jan 16, 2017 • edited

ASP.Net Modules

Getting started

Add Extra Locations

Add Extra Manifest filenames

Additional framework

Asp.Net Mvc

NancyFx

Change Notes for Orchard Team

sebastienros commented Jan 17, 2017

Jetski5822 commented Jan 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtkech Jan 18, 2017 • edited

Choose a reason for hiding this comment

jtkech Jan 18, 2017 • edited

Choose a reason for hiding this comment

jtkech commented Jan 28, 2017

Jetski5822 commented Jan 28, 2017 via email

jtkech commented Jan 28, 2017

jtkech commented Jan 30, 2017

Jetski5822 commented Jan 30, 2017

jtkech commented Feb 1, 2017

Jetski5822 commented Feb 1, 2017

Jetski5822 commented Feb 1, 2017

Choose a reason for hiding this comment

Jetski5822 commented Feb 2, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jetski5822 commented Feb 3, 2017 • edited

Jetski5822 commented Feb 3, 2017

jtkech commented Feb 3, 2017 • edited

Jetski5822 commented Feb 4, 2017

jtkech commented Feb 4, 2017

Jetski5822 commented Feb 13, 2017 via email

sebastienros commented Feb 13, 2017

jtkech commented Feb 14, 2017

jtkech commented Feb 14, 2017

Jetski5822 commented Feb 14, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtkech commented Feb 18, 2017

Jetski5822 commented Feb 18, 2017 via email

jtkech commented Feb 18, 2017 • edited

Jetski5822 commented Feb 23, 2017

jtkech commented Mar 2, 2017

Jetski5822 commented Jan 16, 2017 •

edited

jtkech Jan 18, 2017 •

edited

jtkech Jan 18, 2017 •

edited

Jetski5822 commented Feb 3, 2017 •

edited

jtkech commented Feb 3, 2017 •

edited

jtkech commented Feb 18, 2017 •

edited