Closed datvm closed 1 year ago
After some more thought, I'm a bit conflicted on the part about changing the position of the creator methods.
I am currently working on homogenizing my pattern for wrapping across all my wrapper projects i.e. Blazor.Popper
, Blazor.Streams
, Blazor.FileAPI
, Blazor.DeviceOrientation
, and Blazor.ServiceWorker
.
Some of these projects have no service like Blazor.Streams
or have a service, but not one that is central to the library like IURLService
in Blazor.FileAPI
. If we were to make this change of moving the creator methods then we would need to add services to those projects as well with the only purpose of creating wrapper objects and we would then need to have instances of these services everywhere we need to create objects from those packages which feel too complex.
So I would prefer if we don't add those creator methods in the service and instead make the new internal creator methods on the individual wrapper classes public. So we should also remove the obsolete attributes.
@KristofferStrube I am conflicted about that too. Strictly speaking I chose the current approach just because we can easily use DI for it (without Service we cannot really have access to DI without users passing it to us). Or, we can go with JsonSerializer
approach. Basically user can set the settings individually with each call and optionally set a global (static) default settings.
@KristofferStrube
I have pushed 2 commits fixing issues mentioned in your comment. I also cleaned up the Service class a bit because there was too many repetitive codes that make adding the new parameter tedious. You may need to check:
Your default parameters (like ShowOpenFilePickerAsync(OpenFilePickerOptionsStartInWellKnownDirectory? openFilePickerOptions = null)
) are not accessible because you already have a method with 0 parameter so I removed them (the default = null
) so I can add more overloads.
Any reason FileSystemAccessServiceInProcess
have to extends FileSystemAccessService
? Given the code I don't see why you can't just separate the two. For now I simply explicitly implement IFileSystemAccessService
for FileSystemAccessServiceInProcess
and it contains a lot of boilerplate code.
return serviceCollection
.AddScoped<IFileSystemAccessServiceInProcess, FileSystemAccessServiceInProcess>()
.AddScoped(sp =>
{
var service = sp.GetRequiredService<IFileSystemAccessServiceInProcess>();
return (IFileSystemAccessService)service;
});
However if possible, I think this is better:
return serviceCollection
.AddScoped<IFileSystemAccessServiceInProcess, FileSystemAccessServiceInProcess>()
.AddScoped<IFileSystemAccessService, FileSystemAccessService>();
FileSystemAccessService
and FileSystemAccessServiceInProcess
to internal
because it needs to be consistent with BaseFileSystemAccessService
(I changed all of them to public
now). Though I thought it would be better (?) since we already provided the extension?Also sorry I never used review on GitHub PR before. Do I need to do anything else so you can apply the latest commit, or simply push to that branch and you can merge it?
Just to answer your previous list of comments.
OpenFilePickerOptionsStartInWellKnownDirectory
and OpenFilePickerOptionsStartInFileSystemHandle
into a single type using implicit converters like I have done here in another project. So the methods on the IFileSystemAccessService
will be even slicker then.FileSystemAccessServiceInProcess
extended FileSystemAccessService
was so that we could cast FileSystemAccessServiceInProcess
to FileSystemAccessService
, but I see that you achieved the same using the interfaces and I have tested that they are correctly fetched from DI so that is awesome!FileSystemAccessService
and FileSystemAccessServiceInProcess
public is so that people can potentially inject them however they want in their project. A well-known example of this is people who use Singleton services which would mean that they need all dependant services to also be injected as Singletons whereas our standard is a scoped service.
And I got your PR changes. No need to do anything other that pushing to your branch. :)One other things that I didn't remember to add to my previous review.
IFileSystemAccessServiceInProcess
as they have the same definition as what we new them up to.Awesome! I have pushed a small commit to fixed your last review items. As for the "extra" methods in FileSystemAccessServiceInProcess
, let's just leave them there and fix it when we actually need to change it I guess 😅
This PR improves previous PR to fix #29 . Here're some clarification:
From end-developer perspective:
All
Create
andCreateAsync
methods are markedObsolete
so they should move on from them and use the new methods fromIFileSystemAccessService
instead (for example,FileSystemFileHandle.Create
becomesFileSystemAccessService.CreateFileHandle
).The obsolete methods still work for backward-compatibility but the custom-path feature (and future options) won't work due to missing options reference.
They can also configure the service directly when adding FSA to the
ServiceCollection
:Changes for internal code:
Since a few places still use
Create
andCreateAsync
, I feel it's harder to keep references to the Service instance. Instead those static method now becomesinternal
and the service simply invokes them withOption
instance. This serves 2 purposes: there is no code repetition and we do not need to keep a service class reference on each handler instance.Right now all old (marked
Obsolete
) methods simply point to the newly createdinternal
methods for reusability though withoutOptions
instance certain feature like custom path or anything added in the future won't work.