dnnsoftware
[Enhancement]: Pivot away from ClientDependencyFramework
Is there an existing issue for this?
- [x] I have searched the existing issues
Description of problem
The Client Dependency Framework that DNN uses is a fork of the original project. This project is marked as deprecated and has not had any maintenance for 4 years.
Description of solution
I see one of the following options to get us out of this:
- Maintain CDF ourselves in case issues arise (current situation)
- Look for an alternative (there is a more modern product created by the original maker called Smidge)
- Roll our own
I'd like to propose and expand on 3. The CDF takes care of:
- Keeping a list of JS and CSS files to include.
- Managing where these resources are injected (top or bottom of page)
- Ordering these resources
- Adding a version nr to aid in caching and upgrades
- Minification and compression (optional)
- Bundling (optional)
By default we only use the first 4. 5 and 6 are optional.
Bundling
Bundling has become obsolete with the adoption of HTTP/2. It is also debatable when considering we are a web application framework and what the user needs in terms of assets changes depending on their access and what is installed. This begs the question whether this is something we should offer/pursue.
Minification
I'd like to make the case that minification should really be a concern of the developer and not the framework. In today's world, most developers no longer write plain JS code that is sent to the browser as is. Instead they use frameworks and tools to transpile their work before it goes live.
Without the need to minify or bundle the question arises why we don't do our own CDF with just the aforementioned 4 features. It would greatly simplify matters.
Description of alternatives considered
See text above
Anything else?
No response
Do you plan to contribute code for this enhancement?
- [x] Yes
Would you be interested in sponsoring this enhancement?
- [ ] Yes
Code of Conduct
- [x] I agree to follow this project's Code of Conduct
I totally agree that 5 and 6 is the job of the developer + with http2 and protocol level compression is often of very minimal impact.
Attribute and capability wise, both LINK and SCRIPT have gotten feature rich over time and I suspect they will continue to evolve further in the years to come.
If something new is developed, keep it simple. One suggestion I have from personal experience is to allow the developer to completely describe the resulting HTML while still retaining 1-4 and the ability eliminate dupes from multiple module instances per page.
As a simple example, I wanted defer and async attributes without output like defer="defer" when using DnnNnnInclude. Here is an example that shows a hack to do it; notice the HtmlAttributesAsString? CDF ignores spaces so this hack means the name:value pair injects the name, "async defer crossorigin" which lets me get my desire result. If there was another way to accomplish this I couldn't figure it out. And while I was working on it, I really wanted a way to simply inject/add attributes that would be included verbatim. This would future-proof the implementation even if some of the basic attributes were nicely implemented.
Desired Results that would be easier to do with a verbatim pass-thru and via "helpful" abstractions.
<link href="https://cdn.jsdelivr.net/npm/bootstrap@5/dist/css/bootstrap.min.css?cdv=558" async defer crossorigin="anonymous" rel="stylesheet" type="text/css"/>
<script src="https://kit.fontawesome.com/3c35ca1550.js?cdv=558" async defer crossorigin="anonymous" type="cf8ace8788885bc71fe7c78f-text/javascript"></script>
<script src="https://cdn.jsdelivr.net/npm/@dnncommunity/dnn-elements@0/dist/esm/dnn.js?cdv=558" rel="modulepreload" as="script" type="0b9a417ef5d2689d5049ab6f-module" crossorigin="anonymous"></script>
As we look forward just a few notes. We DO add version # to items by default, so just something to be aware of given the strong caching that is on by default with DNN Platform.
I echo @jeremy-farrance in that getting support for async/defer would be good.
Lastly, if we want to better support things such as content-policy it might be a good place to try and get some help there in identifying sources, allowed sources, etc, to help site admins be able to get things better.
Bundling/Minification - IN many ways I agree is a developer thing, HOWEVER, I think we should consider that we are a platform allowing people to combine multiple things on a single page. The current bundling/minification can have dramatic improvements in performance when using for example 2-3 different third-party solutions on the same page, so we could see major changes, in a negative manner for users in that use-case.
Good point about the Bundling/Minification. Lots of random stuff can get on a page and we at least need to give the developer the control they want+need. I think the obvious solution would be to have the new/upgraded DnnJsInclude(), DnnCssInclude(), etc. have new options for bool AllowBundling, bool AllowMinification.
It also brings up the need for a Host or Portal setting for what the default should be when these params are not set by the developer.
Just adding some terms so this is easier to search for and come back to down the road: Server Settings, Performance, Enable composite files, increment, site version, client resource management, CRM, DependencyHandler, axd
Another feature to pin here if this becomes a real development effort, we need the ability to NOT add the asset if the provided path+file, locally at run-time, does not exist. This should probably be off by default, but that could be debated.
We've had various theme and module projects where an asset might not get generated (e.g. megamenu.bundle.js) and it would be nice to have the DnnJsInclude optionally check for the file's existence and not add the script if its not there.
This prevents two situations. First, the learning mistake where you decide to use MegaMenus and can't get them working because you don't yet know about _preheader.ascx which by default has the DnnJsInclude for megamenu.bundle.js commented out. And second, the theme has it turned on be default but your project isn't using it and is not generating the file and you end up with the sidetrack of figuring out why your DevTools console is throwing a not found on megamenu.bundle.js.
Maybe a param bool VerifyLocalAsset = false and if you set it to true, the missing file does NOT become a link or script in the page source.
The main thrust of my proposal is to get rid of bundling and minification so that we can simplify the requirements to a point where this is feasible to do ourselves. CDF is a dead project and an accident waiting to happen. I am totally comfortable sinking my teeth in a project that does the "management of included resources on a DNN page". So if bundling and minification are requirements this falls outside of that scope for me.
My reply to "The current bundling/minification can have dramatic improvements in performance when using for example 2-3 different third-party solutions on the same page" is: I am sceptical this is true given HTTP2 and standard practice of minifying scripts when distributing modules.
After discussions yesterday in the Git Approvers meeting we agreed that CDF was a security risk in the long run and we need to move away from it sooner rather than later. If it means without bundling and minification than this is acceptable.
There are two common entry points to add resources to the rendering pipeline:
Method 1:
<dnn:DnnJsInclude runat="server" FilePath="js/bootstrap.bundle.min.js" ForceProvider="DnnFormBottomProvider" Priority="100" PathNameAlias="SkinPath" />
Method 2:
ClientResourceManager.RegisterScript(Page, "path/to/my/script.js");
These both go through DotNetNuke.Web.Client to the CDF. Currently they both immediately add the HTML blurb to the web control in Default.aspx. They also add a commented out blurb that can be parsed to resend resources in case the module is cached and this logic doesn't run.
The goals for the new client resource manager are:
- Injectable
- Incorporate new attributes/possibilities of LINK and SCRIPT elements more naturally
- Manage where resources get injected
- Ordering resources
- Adding the cdf version nr at the end of links
Overloads
Focusing on method 2 for now. We now have 10 overloads to add a stylesheet and 10 to add a script. Below are those for stylesheets:
void RegisterStyleSheet(string filePath);
void RegisterStyleSheet(string filePath, IDictionary<string, string> htmlAttributes);
void RegisterStyleSheet(string filePath, int priority);
void RegisterStyleSheet(string filePath, int priority, IDictionary<string, string> htmlAttributes);
void RegisterStyleSheet(string filePath, FileOrder.Css priority);
void RegisterStyleSheet(string filePath, FileOrder.Css priority, IDictionary<string, string> htmlAttributes);
void RegisterStyleSheet(string filePath, int priority, string provider);
void RegisterStyleSheet(string filePath, int priority, string provider, IDictionary<string, string> htmlAttributes);
void RegisterStyleSheet(string filePath, int priority, string provider, string name, string version);
void RegisterStyleSheet(string filePath, int priority, string provider, string name, string version, IDictionary<string, string> htmlAttributes);
If we want to add support for new LINK features (CrossOrigin, FetchPriority, Preload, etc) the amount of overloads might well explode. So I suggest we simplify matters to two overloads: 1 that is the simple "add this resource" and uses defaults for everything and 2 an overload that does everything. For this second overload I see a choice between using parameters like we see above, or we add in an object that encapsulates all of them. I.e.
void RegisterStyleSheet(string filePath, int priority, string provider, string name, string version, CrossOrigin crossOrigin, FetchPriority fetchPriority, bool Preload, ... , IDictionary<string, string> htmlAttributes);
vs
void RegisterStyleSheet(StyleSheetInclude styleSheet);
I have a light preference for the latter as it's always hard to read code with more than let's say 5 parameters. But let me know if you have strong feelings about this.
Name and versions
This is a feature I actually added myself back in the day. It was during the Bootstrap 3 days when many skins began using Bootstrap and as a module developer I wanted to use this as well. Our DNN styling was grossly out of date and didn't cover nearly enough to create the UIs I needed. I ran into issues where multiple Bootstrap css and js files would clash as they originated both from my module as well as a skin. The name/version feature was a way to solve this as both would give their resource the name "bootstrap" and a version nr, and DNN would just send one of the two based on basic alphabetical order of version nr.
Looking back over the time I have come to the conclusion that it was really hard to inform the community about this and few if any used this. And this feature depends on everyone joining in. The incompatibility/breaking changes between Bootstrap 3 and 4 also highlighted the fact that this feature may have a flaw in its reasoning. Since then, Daniel Mettler from 2Sic tried to address the same issue with something he called Koi https://github.com/2sic/connect.koi. This solution allows the module developer to detect which framework is being used and adjust his own code accordingly. But again, this also requires buy in from both module developers and skin developers.
So we have a choice whether to keep using this or deprecate this feature. What are your thoughts on this? Do we keep it as is? Do we incorporate the best ideas from Koi? Or does someone have a better idea?
After re-reading above and looking at a dozen of my own examples in projects since 2019, my opinion has evolved a little and you'll notice, its channeling less initial and ongoing work for the DNN devs...
I always side with "less is more." I struggled with the SkinObjects the last few years because they were too rigid to get modern output; something always missing or needing to be fixed. I always chose to use the methods, and only using the ones with htmlAttributesAsString because I was able to hack in to the attribute name to inject exactly what I wanted. In addition, it allowed me to wrap my code in an if (file exists) {register()} since modern projects have more than a few optionally compiled things (and nobody likes unnecessary console errors missing css and js files).
My point is simple, we need to be able to register a CSS or JS asset and I think its 100% okay to put the responsibility of crafting the entire HTML element and attributes on the developer/designer themselves. Its literally their job anyhow. Its way too complex for the RegisterSomething() method to account for all the current and future options.
In other words, the method needs to handling DNN concerns ONLY, not getting the attributes right for the developer. So that means the method should:
- de-dupe
- path/file
- inject head or body or bottom and order/sort
- add the cdv for cache management
- what else?
Leave the async, defer, rel, as, type, sizes, crossorigin, etc to the dev. I don't even think the Dictionary<string, string> is helpful. Just give me one string HtmlAttribute that I can expect to be injected into the HTML verbatim and let me be responsible for the result. We don't need help with this, DNN shouldn't (accidentally) be opinionated about it, and the DNN dev team shouldn't have to adapt to the changing standards and needs of HTML link and script elements.
I agree with @jeremy-farrance that we don't need special options for HTML attributes that DNN doesn't do anything with. However, I do think using Dictionary<string, string> is more helpful than just a string of attributes because it allows the framework to handle escaping. I think it's important to avoid giving users of the framework an API that makes it easy to accidentally introduce a vulnerability. If Dictionary<string, string> isn't ergonomic enough, we could also support an anonymous object like MVC 5 does (e.g. compare these two APIs).
Regardless of how we handle the attributes, @donker I also agree with you that a parameter object is better than a bunch of raw parameters or overloads. That also gives us the flexibility to add a parameter if needed later, without requiring the addition of an overload.
Regarding names and versions, I think that they're worthwhile, even if only a portion of the community makes use of them. Even within a single custom suite of themes and modules, being able to de-duplicate requests for scripts can be useful. I assume that JavaScript libraries would also still make use of some version of this capability.
I defer to Brian. :)
So if I do this (made up example)
Dictionary<string, string> Attribs = new Dictionary<string, string>();
Attribs.Add("async", string.Empty);
Attribs.Add("rel", "modulepreload");
Attribs.Add("as", "script");
Attribs.Add("type", "module");
Attribs.Add("crossorigin", "anonymous");
var include = new DnnJsInclude
{
FilePath = "//cdn.jsdelivr.net/npm/@dnncommunity/dnn-elements@0/dist/esm/dnn.js",
HtmlAttributes = Attribs,
ForceProvider = "DnnPageHeaderProvider",
Priority = 50,
};
[ register it ]
So the results would look like this, note that async was added without ="" or ="async", and my .Add order was retained:
<script src="https://cdn.jsdelivr.net/npm/@dnncommunity/dnn-elements@0/dist/esm/dnn.js?cdv=556" async rel="modulepreload" as="script" type="module" crossorigin="anonymous"></script>
And finally, I don't have an example, but aren't there use cases where we might need the attribute's values to be wrapped in single quotes instead of double quotes? If yes, how would I specify that? Would there be an optional 3rd param on .Add(name, val, string quote = """") so we have the option to set "'" if double quote is the default?
Also, after typing all that, I realize this stuff might be better to sorted out after the new code/methods exists and we can test and then discuss and code for various use cases.
I would think new Dictionary<string, string> { { "async", string.Empty }, } would provide async="" and new Dictionary<string, string> { { "async", null }, } would produce just async. The order of attributes would not be guaranteed, but in general should be in order for short lists. I would not expect there to be any need to change whether single or double quotes are used (the only scenario in which that would matter would be if the contents include a quote mark, and we would escape those quote marks, rather than switching quotes).
@bdukes - for the single vs double quotes, DNN would just encode the double quotes? I wouldn't have to think and could expect the DNN include/register process to know to do this with my inline JSON data attributes?
<div data-config="{"theme":"dark","showSidebar":true}"></div>
Anyhow, makes sense to me (null vs. empty), thanks! I am looking forward to whatever the next steps are on this.
My objection to stripping out all support from within DNN is that not all developers are born equal. The risk of errors is quite high, it's a ton of code to specify all these parameters and I concur with Brian that I prefer controlled input to avoid injection points. Plus: it takes just one developer to screw it up and your site is toast (the joys of a CMS with extensions). Instead I propose this:
_ourNewManager.RegisterStyleSheet(new StyleSheetInclude {
FilePath = "path/to/file",
Async = true,
Rel = RelOptions.ModulePreload,
Type = TypeOptions.Module
});
Or, maybe even better:
_ourNewManager.RegisterStyleSheet("path/to/file", new StyleSheetOptions {
Async = true,
Rel = RelOptions.ModulePreload,
Type = TypeOptions.Module
});
The latter is a pattern which is quite common. You first do the compulsory parameters and then add the options object which is optional.
A fluent API would also be cool...
_ourNewManager
.RegisterScript()
.FromName("jquery") // vs .FromSrc("path/to/file")
.WithVersion(3.7.1)
.SetAsync() // which defaults to true, but could be .SetAsync(true) or .SetAsync(false)
.SetRel(RelOption.ModulePreload)
.SetType(ScriptTypeOption.Module)
Should new stuff enter specs, we then don't need a bunch of new overloads and can just do extension methods. Also some attributes are only meaningful if some others exist. For instance, crossorigin only has meaning if we have a src attribute for an external script and we can only propose crossorigin if `.FromSrc()' was used, etc. And I think comments on methods are way easier to read in the IDE than parameters, so we can nicely explain in code comments the purpose of each of those fluent apis.
.. that's a good ... that's a good idea. I like that one.
One thing though: the advantage of the very first method is that you could do everything through abstractions and won't require developers to take new dependencies. And I don't think that's possible in this approach.
It maybe important to let the possibility to add custom attributes to scripts and stylesheet link tags. Exemples can be found here : https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Elements/link and https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Elements/script and https://web.dev/articles/defer-non-critical-css?authuser=3&hl=fr Otherwise developers have to bybass CDF for this cases.
Yep, with a fluent API we could have .AddAttributes(Dictionary<string, string> attributes) or .AddAttribute(string name, string value) or something like that
May be we can also manage fonts preloading as used in the default skin of dnn10
That is the idea I'm working towards, yes.
IMO this solution (and in more general the entire API of DNN) should:
- Help developers avoid breaking the site,
- Help stability by helping developers create more secure code,
- Try to cover the 90% use-case
With nr 3 I mean that the aim is not to cover 100% of use cases as that is unattainable and often leads to lots and lots of code. At the end of the day developers can always "hack the system" and go rogue. But at least they know they are now responsible for security.
An opinionated question: this Enhancement appears to be growing exponentially in size and required effort . Wouldn't it better to
Phase 1: do what is required Phase 2: do what is nice to have
I don't disagree. But the nature of developing for the framework is that we don't like to zig-zag our way forward, but rather stay in one direction. So for phase 1 it is important to have an idea what phase 2 might look like.
One feature witch is not mentioned yet, is the ability to remove js or css files. Simimilar with what can be done with the exclude skin object.