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

Refactored markup file loader to be extensible by third-party libraries #1312

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

tomasherceg
Copy link
Member

Currently, it was not possible to extend the AggregateMarkupFileLoader to support special pages (similar to embedded://... format and so on).
I've created AggregateMarkupFileOptions which specifies the types and the order of concrete markup file loaders.
These loaders must be registered in DI, but anyone can add custom loaders like this:

services.Services.Configure<AggregateMarkupFileLoaderOptions>(options =>
{
    // add custom loader to the pipeline
    options.LoaderTypes.Add(typeof(HotReloadMarkupFileLoader));
});
services.Services.AddSingleton<HotReloadMarkupFileLoader>();

Copy link
Member

@exyi exyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of this, I think we can just import IEnumerable<IMarkupFileLoader> from DI. It's harder to control the order of registration that way, but it shouldn't matter in most cases (the loader prefixes should be distinct)

@tomasherceg
Copy link
Member Author

It's not that easy - the Hot Reload is done also by a special loader which basically decorates the default file-system loader.
Maybe we can re-architect this to work differently but I am not sure if this is worth the effort.

@tomasherceg
Copy link
Member Author

I've been looking at this again, and the problem is that the order of services returned by IServiceProvider.GetServices is not guaranteed.

  • The loaders which use a prefix need to be run first.
  • The loader which accesses the file system (the default loader) may be wrapped via the hot reload loader in the debug mode.
  • In tests, we often have our own custom loaders - these basically replace the default loader.

I see two ways how to solve the issue:

  1. The interface IMarkupFileLoader is maybe too universal and not well designed if we have two classes of loaders. We can split it in two: IPrefixedMarkupFileLoader and IDefaultMarkupFileLoader. There can be as many IPrefixedMarkupFileLoaders as the user wants. In that case, we should enforce the uniqueness of the prefix. The IDefaultMarkupFileLoader should be only one and should be used for paths without a prefix.
    It will be a breaking change, but there are not many people who implement their own loaders, and the adaptation to the new interface would not be difficult.

  2. Keep the IMarkupFileLoader interface as is and just add some int Priority field which would be set to int.MaxValue for DefaultMarkupLoader. This way is much easier to implement, but it's not that nice.

@exyi
Copy link
Member

exyi commented Aug 2, 2022

You can keep the AggregateMarkupFileLoaderOptions class, just replace the Type list with IMarkupFileLoader list. It's bit more annoying to use (you need to insert as a specific index), but you can also replace existing loaders

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants