Switch from browserfs to zenfs
#14125
Conversation
|
|
||
| export const BrowserFSInitialization = Symbol('BrowserFSInitialization'); | ||
| export interface BrowserFSInitialization { | ||
| createMountableFileSystem(): Promise<MountableFileSystem> | ||
| initializeFS: (fs: FSModule, provider: BrowserFSFileSystemProvider) => Promise<void>; | ||
| createMountableFileSystem(): Promise<void> | ||
| initializeFS: (provider: BrowserFSFileSystemProvider) => Promise<void>; | ||
| } | ||
|
|
||
| @injectable() | ||
| export class DefaultBrowserFSInitialization implements BrowserFSInitialization { |
There was a problem hiding this comment.
We should rename these symbols, interfaces, classes etc.
| "@zenfs/core": "^0.16.4", | ||
| "@zenfs/dom": "^0.2.14", |
There was a problem hiding this comment.
This change (and the suggested renames) need to be listed as breaking changes in the Changelog
|
|
||
| @injectable() | ||
| export class ExampleBrowserFSInitialization extends DefaultBrowserFSInitialization { | ||
|
|
||
| @inject(EncodingService) | ||
| protected encodingService: EncodingService; | ||
|
|
||
| override async initializeFS(fs: FSModule, provider: BrowserFSFileSystemProvider): Promise<void> { | ||
| override async initializeFS(provider: BrowserFSFileSystemProvider): Promise<void> { |
There was a problem hiding this comment.
I did not look into zenfs. Is this the expected usage? Seems weird to get rid of the filesystem as a parameter as it seems to lose flexibility.
There was a problem hiding this comment.
Is this the expected usage?
Yes, using the fs import/export is correct.
Seems weird to get rid of the filesystem as a parameter as it seems to lose flexibility.
Not using instances for the emulation layer has simplified the internal API a lot since ZenFS doesn't need to worry about handling said instances. ZenFS' configure allows a lot of flexibility.
Additionally, if you'd like to continue using dependency injection, you can still do so. For example:
import type { fs } from '@zenfs/core';
export type FSModule = typeof fs;
export class ExampleFSInitialization {
// ...
initializeFS(fs: FSModule) // ...
// ...
}However, I would not recommend this since it adds unneeded complexity. It may be easier to allow the user to pass in the configuration object instead.
|
I had a closer look at |
|
I don't mean to pressure the team, but ZenFS is now more popular than BrowserFS (due to @robertjndw You may want to try 1.0.0 Release Candidate 0 ( |
|
@james-pre Thank you for your work on We are currently considering several options, including a switch to |
|
I would assume we can close this PR with the addition of #14263? |
Yes, removing the dependency altogether is the better approach for a framework like Theia. Any user who would like to integrate |
What it does
This PR builds on top of the work that was done in PR #12853.
As
browserfswas deprecated in March 24 (see original repo), it is beneficial to switch to the actively maintained forkzenfs(see here).In addition to the active maintenance, the fork also introduces some new features and an improved API (e.g. promise based instead of callback). The new features will simplify the implementation of the missing parts in the
BrowserFSFileSystemProviderHow to test
Build and run the 'browser-only' Theia example.
Similarly to the previously mentioned PR #12853.
Follow-ups
The unimplemented methods of the old PR still exist in this one and will be implemented in a future PR.
Review checklist
Reminder for reviewers