-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Make Extensions.FileProviders supported in browser and only unsupport FSW usage #56189
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
Conversation
|
Tagging subscribers to this area: @maryamariyan, @dotnet/area-extensions-filesystem Issue DetailsFixes #56178 Since
|
safern
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
src/libraries/Microsoft.Extensions.FileProviders.Physical/src/PhysicalFilesWatcher.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.FileProviders.Physical/src/PhysicalFileProvider.cs
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.FileProviders.Physical/src/PhysicalFilesWatcher.cs
Show resolved
Hide resolved
|
This enables the code but none of the tests? Did you try running any of the tests? |
|
I ran them on Windows and Ubuntu. Do you mean enable them for browser? |
|
yes I meant for browser. |
|
I did not run them for browser locally, is it too cumbersome to do? Line 7 in 877af80
|
| FileSystemWatcher watcher = UsePollingFileWatcher && UseActivePolling ? null : | ||
| #if NETCOREAPP | ||
| OperatingSystem.IsBrowser() ? throw new PlatformNotSupportedException(SR.Format(SR.FileSystemWatcher_PlatformNotSupported, typeof(FileSystemWatcher))) : new FileSystemWatcher(root); | ||
| #else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this still throw PNSE when creating a PhysicalFileProvider on browser? Shouldn't the logic be something like:
(UsePollingFileWatcher && UseActivePolling) || OperatingSystem.IsBrowser() ? null : ...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see, so we should always fallback to use polling on browser. That makes sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixes #56178
Since
PhysicalFilesWatchercan be used without aFileSystemWatcherthrough polling, I reduced the unsupported scenarios to only the ones that actually use FSW in browser with a couple of runtime checks.Let me know if this is not a good idea and instead we should mark the whole type as unsupported.