Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Switch from browserfs to zenfs #14125

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Switch from browserfs to zenfs #14125

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c27de31
Switch to zenfs for browser-only implementation
robertjndw Sep 2, 2024
d116e65
Fix async with new API
robertjndw Sep 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions examples/api-samples/src/browser-only/filesystem/example-filesystem-initialization.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,15 @@ import { inject, injectable, interfaces } from '@theia/core/shared/inversify';
import { EncodingService } from '@theia/core/lib/common/encoding-service';
import { BrowserFSInitialization, DefaultBrowserFSInitialization } from '@theia/filesystem/lib/browser-only/browserfs-filesystem-initialization';
import { BrowserFSFileSystemProvider } from '@theia/filesystem/lib/browser-only/browserfs-filesystem-provider';
import type { FSModule } from 'browserfs/dist/node/core/FS';
import { fs } from '@zenfs/core';

@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> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@james-pre james-pre Sep 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sdirix,

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.

try {
if (!fs.existsSync('/home/workspace')) {
await provider.mkdir(new URI('/home/workspace'));
Expand Down
3 changes: 2 additions & 1 deletion packages/filesystem/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@
"@types/tar-fs": "^1.16.1",
"async-mutex": "^0.3.1",
"body-parser": "^1.18.3",
"browserfs": "^1.4.3",
"@zenfs/core": "^0.16.4",
"@zenfs/dom": "^0.2.14",
Comment on lines +13 to +14
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change (and the suggested renames) need to be listed as breaking changes in the Changelog

"http-status-codes": "^1.3.0",
"minimatch": "^5.1.0",
"multer": "1.4.4-lts.1",
Expand Down
39 changes: 10 additions & 29 deletions packages/filesystem/src/browser-only/browserfs-filesystem-initialization.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,48 +14,29 @@
// SPDX-License-Identifier: EPL-2.0 OR GPL-2.0-only WITH Classpath-exception-2.0
// *****************************************************************************

import type { FSModule } from 'browserfs/dist/node/core/FS';
import type { BrowserFSFileSystemProvider } from './browserfs-filesystem-provider';
import { injectable } from '@theia/core/shared/inversify';
import { FileSystem, initialize } from 'browserfs';
import MountableFileSystem from 'browserfs/dist/node/backend/MountableFileSystem';
import { configure } from '@zenfs/core';
import { IndexedDB } from '@zenfs/dom';

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 {
Comment on lines 21 to 29
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should rename these symbols, interfaces, classes etc.


createMountableFileSystem(): Promise<MountableFileSystem> {
return new Promise(resolve => {
FileSystem.IndexedDB.Create({}, (e, persistedFS) => {
if (e) {
throw e;
}
if (!persistedFS) {
throw Error('Could not create filesystem');
}
FileSystem.MountableFileSystem.Create({
'/home': persistedFS

}, (error, mountableFS) => {
if (error) {
throw error;
}
if (!mountableFS) {
throw Error('Could not create filesystem');
}
initialize(mountableFS);
resolve(mountableFS);
});
});
createMountableFileSystem(): Promise<void> {
return configure({
mounts: {
'/home': IndexedDB,
}
});
}

async initializeFS(fs: FSModule, provider: BrowserFSFileSystemProvider): Promise<void> {
async initializeFS(provider: BrowserFSFileSystemProvider): Promise<void> {

}
}
52 changes: 20 additions & 32 deletions packages/filesystem/src/browser-only/browserfs-filesystem-provider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,10 @@ import {
import { Event, URI, Disposable, CancellationToken } from '@theia/core';
import { TextDocumentContentChangeEvent } from '@theia/core/shared/vscode-languageserver-protocol';
import { ReadableStreamEvents } from '@theia/core/lib/common/stream';
import { BFSRequire } from 'browserfs';
import type { FSModule } from 'browserfs/dist/node/core/FS';
import type { FileSystem } from 'browserfs/dist/node/core/file_system';
import MountableFileSystem from 'browserfs/dist/node/backend/MountableFileSystem';
import { basename, dirname, normalize } from 'path';
import Stats from 'browserfs/dist/node/core/node_fs_stats';
import { retry } from '@theia/core/lib/common/promise-util';
import { BrowserFSInitialization } from './browserfs-filesystem-initialization';
import { fs, Stats } from '@zenfs/core';

// adapted from DiskFileSystemProvider
@injectable()
Expand All @@ -53,15 +49,12 @@ export class BrowserFSFileSystemProvider implements FileSystemProviderWithFileRe
private writeHandles: Set<number> = new Set();
private canFlush: boolean = true;

private fs: FSModule;
private mountableFS: MountableFileSystem;
private initialized: Promise<true>;

constructor(@inject(BrowserFSInitialization) readonly initialization: BrowserFSInitialization) {
const init = async (): Promise<true> => {
this.mountableFS = await initialization.createMountableFileSystem();
this.fs = BFSRequire('fs');
await initialization.initializeFS(this.fs, new Proxy(this, {
await initialization.createMountableFileSystem();
await initialization.initializeFS(new Proxy(this, {
get(target, prop, receiver): unknown {
if (prop === 'initialized') {
return Promise.resolve(true);
Expand All @@ -74,11 +67,6 @@ export class BrowserFSFileSystemProvider implements FileSystemProviderWithFileRe
this.initialized = init();
}

async mount(mountPoint: string, fs: FileSystem): Promise<void> {
await this.initialized;
this.mountableFS.mount(mountPoint, fs);
};

watch(_resource: URI, _opts: WatchOptions): Disposable {
return Disposable.NULL;
}
Expand All @@ -88,7 +76,7 @@ export class BrowserFSFileSystemProvider implements FileSystemProviderWithFileRe

let stats: Stats;
try {
stats = await this.promisify(this.fs.stat)(path) as Stats;
stats = await fs.promises.stat(path) as Stats;
} catch (error) {
throw this.toFileSystemProviderError(error);
}
Expand All @@ -107,7 +95,7 @@ export class BrowserFSFileSystemProvider implements FileSystemProviderWithFileRe
async mkdir(resource: URI): Promise<void> {
await this.initialized;
try {
await this.promisify(this.fs.mkdir)(this.toFilePath(resource));
await fs.promises.mkdir(this.toFilePath(resource));
} catch (error) {
throw this.toFileSystemProviderError(error);
}
Expand All @@ -116,7 +104,7 @@ export class BrowserFSFileSystemProvider implements FileSystemProviderWithFileRe
await this.initialized;
try {

const children = await this.promisify(this.fs.readdir)(this.toFilePath(resource)) as string[];
const children = await fs.promises.readdir(this.toFilePath(resource)) as string[];
const result: [string, FileType][] = [];
await Promise.all(children.map(async child => {
try {
Expand All @@ -136,7 +124,7 @@ export class BrowserFSFileSystemProvider implements FileSystemProviderWithFileRe
await this.initialized;
// FIXME use options
try {
await this.promisify(this.fs.unlink)(this.toFilePath(resource));
await fs.promises.unlink(this.toFilePath(resource));
} catch (error) {
throw this.toFileSystemProviderError(error);
}
Expand All @@ -150,28 +138,28 @@ export class BrowserFSFileSystemProvider implements FileSystemProviderWithFileRe
}
try {
// assume FS is path case sensitive - correct?
const targetExists = await this.promisify(this.fs.exists)(toFilePath);
const targetExists = await fs.promises.exists(toFilePath);
if (targetExists) {
throw Error(`File '${toFilePath}' already exists.`);
}
if (fromFilePath === toFilePath) {
return Promise.resolve();
}

await this.promisify(this.fs.rename)(fromFilePath, toFilePath);
await fs.promises.rename(fromFilePath, toFilePath);

const stat = await this.promisify(this.fs.lstat)(toFilePath) as Stats;
const stat = await fs.promises.lstat(toFilePath) as Stats;
if (stat.isDirectory() || stat.isSymbolicLink()) {
return Promise.resolve(); // only for files
}
const fd = await this.promisify(open)(toFilePath, 'a');
const file_handle = await fs.promises.open(toFilePath, 'a');
try {
await this.promisify(this.fs.futimes)(fd, stat.atime, new Date());
await file_handle.utimes(stat.atime, new Date());
} catch (error) {
// ignore
}

this.promisify(this.fs.close)(fd);
file_handle.close();
} catch (error) {
// rewrite some typical errors that can happen especially around symlinks
// to something the user can better understand
Expand All @@ -190,7 +178,7 @@ export class BrowserFSFileSystemProvider implements FileSystemProviderWithFileRe
await this.initialized;
try {
const filePath = this.toFilePath(resource);
return await this.promisify(this.fs.readFile)(filePath) as Uint8Array;
return await fs.promises.readFile(filePath) as Uint8Array;
} catch (error) {
throw this.toFileSystemProviderError(error);
}
Expand All @@ -203,7 +191,7 @@ export class BrowserFSFileSystemProvider implements FileSystemProviderWithFileRe

// Validate target unless { create: true, overwrite: true }
if (!opts.create || !opts.overwrite) {
const fileExists = await this.promisify(this.fs.exists)(filePath);
const fileExists = await fs.promises.exists(filePath);
if (fileExists) {
if (!opts.overwrite) {
throw createFileSystemProviderError('File already exists', FileSystemProviderErrorCode.FileExists);
Expand Down Expand Up @@ -251,7 +239,7 @@ export class BrowserFSFileSystemProvider implements FileSystemProviderWithFileRe
flags = 'r';
}

const handle = await this.promisify(this.fs.open)(filePath, flags) as number;
const handle = (await fs.promises.open(filePath, flags)).fd;

// remember this handle to track file position of the handle
// we init the position to 0 since the file descriptor was
Expand Down Expand Up @@ -279,7 +267,7 @@ export class BrowserFSFileSystemProvider implements FileSystemProviderWithFileRe
// to flush the contents to disk if possible.
if (this.writeHandles.delete(fd) && this.canFlush) {
try {
await this.promisify(this.fs.fdatasync)(fd);
await this.promisify(fs.fdatasync)(fd);
} catch (error) {
// In some exotic setups it is well possible that node fails to sync
// In that case we disable flushing and log the error to our logger
Expand All @@ -288,7 +276,7 @@ export class BrowserFSFileSystemProvider implements FileSystemProviderWithFileRe
}
}

await this.promisify(this.fs.close)(fd);
await this.promisify(fs.close)(fd);
}
async read(fd: number, pos: number, data: Uint8Array, offset: number, length: number): Promise<number> {
await this.initialized;
Expand All @@ -297,7 +285,7 @@ export class BrowserFSFileSystemProvider implements FileSystemProviderWithFileRe
let bytesRead: number | null = null;
try {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const result: { bytesRead: number, buffer: Uint8Array } | number = (await this.promisify(this.fs.read)(fd, data, offset, length, normalizedPos)) as any;
const result: { bytesRead: number, buffer: Uint8Array } | number = (await this.promisify(fs.read)(fd, data, offset, length, normalizedPos)) as any;

if (typeof result === 'number') {
bytesRead = result; // node.d.ts fail
Expand Down Expand Up @@ -327,7 +315,7 @@ export class BrowserFSFileSystemProvider implements FileSystemProviderWithFileRe
let bytesWritten: number | null = null;
try {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const result: { bytesWritten: number, buffer: Uint8Array } | number = (await this.promisify(this.fs.write)(fd, data, offset, length, normalizedPos)) as any;
const result: { bytesWritten: number, buffer: Uint8Array } | number = (await this.promisify(fs.write)(fd, data, offset, length, normalizedPos)) as any;

if (typeof result === 'number') {
bytesWritten = result; // node.d.ts fail
Expand Down
Loading
Loading