-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Switch from browserfs to OPFS #14263
base: master
Are you sure you want to change the base?
Conversation
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.
Thanks for the work ❤️ ! For me it mostly works ;)
Please have a look at my comments and make sure that all automated CI checks go through.
examples/api-samples/src/browser-only/filesystem/example-filesystem-initialization.ts
Outdated
Show resolved
Hide resolved
createMountableFileSystem(): Promise<FileSystemDirectoryHandle> | ||
initializeFS: (fs: FileSystemDirectoryHandle, provider: OPFSFileSystemProvider) => Promise<void>; |
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.
We are now exclusively using OPFS, therefore the whole initialization could be adapted, for example like this
createMountableFileSystem(): Promise<FileSystemDirectoryHandle> | |
initializeFS: (fs: FileSystemDirectoryHandle, provider: OPFSFileSystemProvider) => Promise<void>; | |
getRootDirectory(): Promise<FileSystemDirectoryHandle> |
The initialization can then just be implemented in this one method. What do you think?
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.
I see your point about simplifying the initialization into one method now that we're exclusively using OPFS. However, I think keeping the two methods still makes sense in this context.
One focuses on returning the root directory, while the other is responsible for preparing the file system, which is especially useful when we want to create files and folders in advance for the example. What are your thoughts on this approach?
packages/filesystem/src/browser-only/opfs-filesystem-provider.ts
Outdated
Show resolved
Hide resolved
packages/filesystem/src/browser-only/opfs-filesystem-provider.ts
Outdated
Show resolved
Hide resolved
packages/filesystem/src/browser-only/opfs-filesystem-provider.ts
Outdated
Show resolved
Hide resolved
packages/filesystem/src/browser-only/opfs-filesystem-provider.ts
Outdated
Show resolved
Hide resolved
packages/filesystem/src/browser-only/opfs-filesystem-provider.ts
Outdated
Show resolved
Hide resolved
packages/filesystem/src/browser-only/opfs-filesystem-provider.ts
Outdated
Show resolved
Hide resolved
packages/filesystem/src/browser-only/opfs-filesystem-provider.ts
Outdated
Show resolved
Hide resolved
@sdirix Can you maybe take a look at the CI/CD error? It seems that with the addition of |
Seems like the Typescript Eslint integration does not like the Anyway, you can override the lib settings in "lib": [
"ES2019",
"ES2020.Promise",
"DOM"
] That should fix the issue |
LGTM! @robertjndw any updates on this? |
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.
I updated to latest master
.
I have some more comments before we can merge.
What it does
This PR builds on top of the work that was done in PR #12853.
As browserfs was deprecated in March 24 (see original repo), it is beneficial to switch to an alternative.
As discussed in #14125, the direct successor of browserfs is zenfs. However, during the research I also stumbled across OPFS which is a new native Browser API which covers a lot of the features necessary for this use case.
How to test
Build and run the 'browser-only' Theia example.
Similarly to the previously mentioned PR #12853.
For debugging purposes of the OPFS, I can really recommend this extension for the Chrome DevTools: https://github.com/tomayac/opfs-explorer
Follow-ups
The watch functionality is currently missing. Since OPFS does not provide a native API for this, it has to be manually tracked and implemented
Review checklist
Reminder for reviewers