KristofferStrube / Blazor.FileSystemAccess

A Blazor wrapper for the File System Access browser API.
https://kristofferstrube.github.io/Blazor.FileSystemAccess/
MIT License
327 stars 39 forks source link

Module import with _content path does not work for Blazor with Chrome Extension #29

Closed datvm closed 1 year ago

datvm commented 1 year ago

I am building a Chrome extension and due to their file system, any folder starting with _ is prohibited. I am using Blazor.BrowserExtension to build Blazor project and there is similar issue mingyaulee/Blazor.BrowserExtension#43, and the _content folder is renamed to content instead.

My suggestion is to add support for custom module path so we can put our own path. The file in question is: https://github.com/KristofferStrube/Blazor.FileSystemAccess/blob/d587c50baad45a04b30fd95cbd48af611e165ce9/src/KristofferStrube.Blazor.FileSystemAccess/Extensions/IJSRuntimeExtensions.cs#L7

KristofferStrube commented 1 year ago

Interesting problem. Do I understand correctly that your intention is to copy the content of the script to some path that does not contain an underscore manually? And then referencing that?

datvm commented 1 year ago

Hi, the other library does that automatically on build (renaming _content to content). But yes it's correct that the path is no longer _content/... but content/..., due to _content being an illegal name in Chrome extension.

Sorry I should not have posted late Saturday evening, now that I read it again myself I barely made sense myself 😅

KristofferStrube commented 1 year ago

I will try to make some configurations possible where you can customize the route to the helper script.

datvm commented 1 year ago

@KristofferStrube hi, I added a pull request that add an option class using Options pattern. However since the FileHandlers may be created without the service itself and have no access to the DI, I added overloads to the Create and CreateAsync methods so it wouldn't be breaking change (constructors are already internal). Now each Handler class would have these:

    public static new FileSystemFileHandle Create(IJSRuntime jSRuntime, IJSObjectReference jSReference, IServiceProvider services)
    {
        return Create(jSRuntime, jSReference, services, Create);
    }

    public static new FileSystemFileHandle Create(IJSRuntime jSRuntime, IJSObjectReference jSReference)
    {
        return Create(jSRuntime, jSReference, Create);
    }

    public static new FileSystemFileHandle Create(IJSRuntime jSRuntime, IJSObjectReference jSReference, FileSystemAccessOptions options)
    {
        return new(jSRuntime, jSReference, options);
    }

If options is not specified, the default path is used. I give IServiceProvider services overload because IOption<FileSystemAccessOptions> is a very long name (and requires 2 additional namespaces) to inject. If user uses custom path, when using Create or CreateAsync they have to specify options or services (IServiceProvider).

Setup:

// Use this configuration to use custom path
builder.Services.Configure<FileSystemAccessOptions>(opt =>
{
    // In this example, this path is copied directly to the wwwroot folder
    opt.ScriptPath = $"/content/{FileSystemAccessOptions.DefaultNamespace}/{FileSystemAccessOptions.DefaultNamespace}.js";
});

Please tell me if there is any issue with this solution. Thanks a lot and hope this is pushed soon :)

KristofferStrube commented 1 year ago

Awesome. Thanks for the PR. I will try to review it tomorrow.

One concern I have is that we use two other libraries Blazor.Streams and Blazor.FileAPI that follow the same helperTask pattern. These would have the same problem as you are currently facing if you are to use the FileSystemWritableFileStream or Blobs exposed in this package. I will have to look at how we will handle this.

My own approach would have been to use a Static property that could be set as I imagine that very few will use this feature. But I agree that this is probably a good fit for using an options class as it supports the possibility of adding any other configuration in the future.

datvm commented 1 year ago

@KristofferStrube

One concern I have is that we use two other libraries Blazor.Streams and Blazor.FileAPI that follow the same helperTask pattern.

Oh I am not aware it uses those libraries. As for fixing for all the libraries, unfortunately I think we have to do that for all dependencies as the "upstream" library (mingyaulee/Blazor.BrowserExtension) cannot handle it as per referred issue. Also the blog post seems to suggest leaving the JS path to end-developer instead of the current approach which may break in the future or in different environment like what I am facing.

Btw after submitting the PR, I just remembered the way I approach it is not exactly the best since I had to pass the Option class or IServiceProvider everywhere. Instead I should have moved the method to the FileSystemAccessService class (i.e. the proper way of creating an instance of File/Directory FileHandler should be through the service instead of the static Create/CreateAsync methods), and leave the current Create one intact for backward-compatibility though you won't be able to use custom path for those old ones. I will submit another PR later with this better approach.

My own approach would have been to use a Static property that could be set as I imagine that very few will use this feature. But I agree that this is probably a good fit for using an options class as it supports the possibility of adding any other configuration in the future.

Yes this will make things harder in the future though admittedly it's much easier now. It's part of .NET DI as well so it's more "unified" and streamlined. In the new PR I will also introduce an overload so developers can do the config right within the AddFileSystemAccessService call if they wish to.


Overall, thank you so much for this awesome project and I see you have other great Blazor projects as well. I am happy to help improving them further!

datvm commented 1 year ago

I pushed the new PR. Please review it when you are available :)

This PR however does add Obsolete attributes to previous methods. The detailed explanation is in that PR. I believe this is for the best interest of the project in the future but I am open to go through other route if you find this too much of a change.

KristofferStrube commented 1 year ago

I will push Release 2.1.0 to NuGet in a bit. :)

datvm commented 1 year ago

Thank you so much! That's awesome. Time to continue my weekend side project!

datvm commented 1 year ago

For future projects, I asked the Blazor team for better solution so we don't have to copy this to all RCL projects. The answer was no so I wrote https://github.com/datvm/Blazor.JsRuntimeRedirect. This way special needs like Chrome Extension can be handled from a single library instead of each library author :)