Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OSOE-415: Orchard-Base-Theme shouldn't lock on every page load #52

Merged
merged 23 commits into from
Dec 20, 2022
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
9821b1a
Rewrite RemoveBootstrap5 to only lock when there is actually anything…
sarahelsaig Dec 8, 2022
41a7bcc
Make it a middleware instead.
sarahelsaig Dec 9, 2022
d161f2a
Don't async when done.
sarahelsaig Dec 9, 2022
0857456
Elaborate the xmldoc.
sarahelsaig Dec 9, 2022
6910f09
Update Lombiq.BaseTheme/Middlewares/RemoveBootstrapMiddleware.cs
sarahelsaig Dec 11, 2022
f9e2c45
Merge remote-tracking branch 'origin/dev' into issue/OSOE-415
sarahelsaig Dec 18, 2022
e85ffbe
Update site.css.map.
sarahelsaig Dec 18, 2022
500414b
Remove _done, because of false positive in multi-tenancy.
sarahelsaig Dec 18, 2022
5d4f6d0
Add notes about tenants.
sarahelsaig Dec 18, 2022
bd0a862
Add version to the bootstrap script so it doesn't break BehaviorChart…
sarahelsaig Dec 18, 2022
2c41be1
Apply suggestions from code review
sarahelsaig Dec 19, 2022
795417d
Remove only V5.
sarahelsaig Dec 19, 2022
f48a11e
Fix InvalidOperationException.
sarahelsaig Dec 19, 2022
3926f5b
Keep the latest version of script.
sarahelsaig Dec 20, 2022
c1f3f1f
simplify
sarahelsaig Dec 20, 2022
5217fd2
Upgrade bootstrap to 5.2.3.
sarahelsaig Dec 20, 2022
b67b2ac
Update wwwroot.
sarahelsaig Dec 20, 2022
2095f43
Fix BS 5.2 navbar breaking change.
sarahelsaig Dec 20, 2022
e07e84b
Update bootstrap version in samples project too.
sarahelsaig Dec 20, 2022
39964c7
Fix bug with RemoveBootstrapMiddleware.
sarahelsaig Dec 20, 2022
22235a8
Use the JS bundle (contains popper).
sarahelsaig Dec 20, 2022
effdaaa
Add comment in GetResourcesToClear.
sarahelsaig Dec 20, 2022
c76f164
Tighten relationship between references to NPM BS version.
sarahelsaig Dec 20, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion Lombiq.BaseTheme.Samples/wwwroot/css/site.css.map

Large diffs are not rendered by default.

28 changes: 0 additions & 28 deletions Lombiq.BaseTheme/Extensions/ResourceManagementOptionsExtensions.cs

This file was deleted.

4 changes: 3 additions & 1 deletion Lombiq.BaseTheme/Manifest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,7 @@
Author = "Lombiq Technologies",
Version = "0.0.1",
Website = "https://github.com/Lombiq/Orchard-Base-Theme",
Description = "The base frontend theme for shared content that are not specific to a specific project's theme."
Description = "The base frontend theme for shared code that is not specific to a specific project's theme." +
"Warning: themes using this as the base remove the stock Bootstrap resource. If you switch to a different " +
"theme, please reload the tenant from Configuration → Tenants in the admin menu."
)]
76 changes: 76 additions & 0 deletions Lombiq.BaseTheme/Middlewares/RemoveBootstrapMiddleware.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
using Lombiq.BaseTheme.Constants;
using Microsoft.AspNetCore.Http;
using Microsoft.Extensions.Options;
using OrchardCore.DisplayManagement.Manifest;
using OrchardCore.Environment.Extensions;
using OrchardCore.ResourceManagement;
using OrchardCore.Themes.Services;
using OrchardCore.Workflows.Helpers;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;

namespace Lombiq.BaseTheme.Middlewares;

/// <summary>
/// Removes the built-in Bootstrap resource if the currently selected theme uses Lombiq.BaseTheme as its base theme.
/// Themes derived from Lombiq.BaseTheme use Bootstrap from NPM as it's compiled into their site stylesheet. So this
/// duplicate resource is not needed and can cause problems if not removed. This situation can arise when a module
/// (such as Lombiq.DataTables) depends on Bootstrap and doesn't explicitly depend on Lombiq.BaseTheme so the built-in
/// resource would be injected if this middleware didn't remove it.
/// </summary>
public class RemoveBootstrapMiddleware
{
private static readonly object _lock = new();
private static readonly string[] _resourceTypes = { "stylesheet", "script" };

private readonly RequestDelegate _next;

public RemoveBootstrapMiddleware(RequestDelegate next) => _next = next;

public async Task InvokeAsync(
HttpContext context,
IOptions<ResourceManagementOptions> resourceManagementOptions,
ISiteThemeService siteThemeService)
{
if (IsCurrentTheme(await siteThemeService.GetSiteThemeAsync()))
{
var resourcesToClear = resourceManagementOptions
.Value
.ResourceManifests
.Where(manifest => !manifest.GetResources("$" + nameof(FeatureIds.Area)).ContainsKey(FeatureIds.Area))
.SelectMany(manifest => _resourceTypes
.SelectWhere(manifest.GetResources)
.SelectWhere(
resources => resources.GetMaybe("bootstrap"),
resource => resource?.Any(IsVersion5) == true))
.ToList();

// This only happens once per tenant process instance, so locking is rare.
if (resourcesToClear.Any()) ClearResources(resourcesToClear);
}

await _next(context);
}

private static bool IsCurrentTheme(IExtensionInfo currentSiteTheme) =>
currentSiteTheme?.Id == FeatureIds.BaseTheme ||
(currentSiteTheme?.Manifest.ModuleInfo as ThemeAttribute)?.BaseTheme == FeatureIds.BaseTheme;

private static bool IsVersion5(ResourceDefinition definition) =>
Version.TryParse(definition.Version, out var version) &&
version.Major == 5;

private static void ClearResources(IEnumerable<IList<ResourceDefinition>> resourcesToClear)
{
lock (_lock)
{
foreach (var resource in resourcesToClear)
{
var toRemove = resource.Where(IsVersion5).ToList();
resource.RemoveRange(toRemove);
}
}
}
}
3 changes: 2 additions & 1 deletion Lombiq.BaseTheme/ResourceManagementOptionsConfiguration.cs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ static ResourceManagementOptionsConfiguration()

_manifest
.DefineScript("bootstrap")
.SetUrl(Vendors + "bootstrap/js/bootstrap.min.js", Vendors + "bootstrap/js/bootstrap.js");
.SetUrl(Vendors + "bootstrap/js/bootstrap.min.js", Vendors + "bootstrap/js/bootstrap.js")
.SetVersion("5.1.3");
}

public void Configure(ResourceManagementOptions options) => options.ResourceManifests.Add(_manifest);
Expand Down
7 changes: 7 additions & 0 deletions Lombiq.BaseTheme/Startup.cs
Original file line number Diff line number Diff line change
@@ -1,13 +1,17 @@
using Lombiq.BaseTheme.Middlewares;
using Lombiq.BaseTheme.Migrations;
using Lombiq.BaseTheme.Services;
using Lombiq.DataTables.Navigation;
using Lombiq.HelpfulLibraries.OrchardCore.ResourceManagement;
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Mvc;
using Microsoft.AspNetCore.Routing;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Options;
using OrchardCore.Data.Migration;
using OrchardCore.Modules;
using OrchardCore.ResourceManagement;
using System;

namespace Lombiq.BaseTheme;

Expand All @@ -23,4 +27,7 @@ public override void ConfigureServices(IServiceCollection services)

services.AddScoped<IResourceFilterProvider, ResourceFilters>();
}

public override void Configure(IApplicationBuilder app, IEndpointRouteBuilder routes, IServiceProvider serviceProvider) =>
app.UseMiddleware<RemoveBootstrapMiddleware>();
}
8 changes: 0 additions & 8 deletions Lombiq.BaseTheme/Views/Layout.cshtml
Original file line number Diff line number Diff line change
@@ -1,17 +1,9 @@
<!DOCTYPE html>
@using Microsoft.Extensions.Options
@using OrchardCore.ResourceManagement

@inject ICssClassHolder CssClassHolder
@inject IOptions<ResourceManagementOptions> ResourceManagementOptions

<html lang="@Orchard.CultureName()">
<head>
@{
// Remove all external Bootstrap 5 resources. (This way we can update Bootstrap independent of OC.)
ResourceManagementOptions.RemoveBootstrap5();
}

<meta charset="utf-8">
<meta name="viewport" content="width=device-width, initial-scale=1.0" />
<title><shape type="PageTitle"></shape></title>
Expand Down
2 changes: 1 addition & 1 deletion Lombiq.BaseTheme/wwwroot/css/site.css.map

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion Readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ Use this as the base theme of any custom frontend themes you create. For instruc

The theme makes use of the [ICssClassHolder](Services/ICssClassHolder.cs) service which provides a scoped container for adding class names from your own code. Use the provided zone names in the [ZoneNames](Constants/ZoneNames.cs) static class to address it.

You may have noticed, that we mentioned Bootstrap 5.1, even though your version of Orchard Core may be still using Bootstrap 5.0. This theme automatically removes the built-in Bootstrap resource manifests and replaces them with the vendor's JavaScript file pulled from NPM. As the Bootstrap stylesheet is already bundled into the site stylesheet there is no need to include that in the resource manifest.
You may have noticed, that we mentioned Bootstrap 5.1, even though your version of Orchard Core may be still using Bootstrap 5.0. This theme automatically removes the built-in Bootstrap resource manifests on the current tenant and replaces them with the vendor's JavaScript file pulled from NPM. As the Bootstrap stylesheet is already bundled into the site stylesheet there is no need to include that in the resource manifest. If you want to switch over to a different theme that doesn't use this as its base, please reload your tenant by going to Admin → Configuration → Tenants and clicking on the current tenant's Reload button.

Besides the style and layout, the theme also automatically includes a minimalist helper script that eases transition away from jQuery. You don't really need full jQuery now that Internet Explorer is effectively dead (Internet Explorer 11 is going end of life on June 15, 2022 so you should not support it in any new project at this time). The script gives you the `window.helper` object. You can use `helper.ready(($) => {})` in your scripts, where `$(querySelector, baseElement)` returns a JavaScript `Array` of `Element`s.

Expand Down