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.

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

Merged
merged 16 commits into from
Nov 28, 2024
Merged

Conversation

robertjndw
Copy link
Contributor

@robertjndw robertjndw commented Oct 7, 2024

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

@robertjndw robertjndw changed the title Feat/switch to opfs Switch from browserfs to OPFS Oct 7, 2024
@JonasHelming JonasHelming requested a review from sdirix October 8, 2024 06:41
@msujew msujew mentioned this pull request Oct 8, 2024
1 task
Copy link
Member

@sdirix sdirix left a 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.

packages/filesystem/package.json Show resolved Hide resolved
Comment on lines 22 to 23
createMountableFileSystem(): Promise<FileSystemDirectoryHandle>
initializeFS: (fs: FileSystemDirectoryHandle, provider: OPFSFileSystemProvider) => 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.

We are now exclusively using OPFS, therefore the whole initialization could be adapted, for example like this

Suggested change
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?

Copy link
Contributor Author

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?

@sdirix sdirix mentioned this pull request Oct 9, 2024
@james-pre
Copy link

Could a maintainer please link this to #13520 so #13520 will be auto-closed on merge? Thanks!

@sdirix sdirix linked an issue Oct 11, 2024 that may be closed by this pull request
@robertjndw
Copy link
Contributor Author

@sdirix Can you maybe take a look at the CI/CD error? It seems that with the addition of dom.asynciterable to the tsconfig something broke 🤔

@sdirix
Copy link
Member

sdirix commented Oct 16, 2024

@sdirix Can you maybe take a look at the CI/CD error? It seems that with the addition of dom.asynciterable to the tsconfig something broke 🤔


@theia/ffmpeg: $ eslint --cache=true --no-error-on-unmatched-pattern=true "{src,test}/**/*.{ts,tsx}"
@theia/re-exports:
@theia/re-exports: /home/runner/work/theia/theia/dev-packages/private-re-exports/src/bin-package-re-exports-from-package.ts
@theia/re-exports: 0:0 error Parsing error: Invalid value for lib provided: dom.asynciterable


Seems like the Typescript Eslint integration does not like the dom.asynciterable lib. I'm not sure why this is the case as the Typescript we are using is definitely compatible to it.

Anyway, you can override the lib settings in configs/base.eslintrc.json and there specify the parserOptions.lib to the previous values, i.e.

    "lib": [
      "ES2019",
      "ES2020.Promise",
      "DOM"
    ]

That should fix the issue

@james-pre
Copy link

LGTM! @robertjndw any updates on this?

Copy link
Member

@sdirix sdirix left a 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.

@tsmaeder
Copy link
Contributor

@sdirix what's the state on this one?

@sdirix
Copy link
Member

sdirix commented Nov 28, 2024

@tsmaeder I have to review the latest changes (which were pushed yesterday)

Copy link
Member

@sdirix sdirix left a comment

Choose a reason for hiding this comment

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

Looks good to me! Great work!

@sdirix sdirix merged commit 717c063 into eclipse-theia:master Nov 28, 2024
11 checks passed
@github-actions github-actions bot added this to the 1.56.0 milestone Nov 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

BrowserFS Deprecated
4 participants