Skip to content

feat: Add origin private file system scaffold #5758

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

Merged
merged 6 commits into from
Mar 18, 2025
Merged

Conversation

Eason0729
Copy link
Contributor

Which issue does this PR close?

close #2442

Rationale for this change

What changes are included in this PR?

read/write support for OPFS

Are there any user-facing changes?

@Eason0729
Copy link
Contributor Author

Thank you for your review(and I missing it earlier).

I will address your suggestions tomorrow.

@Eason0729 Eason0729 marked this pull request as ready for review March 14, 2025 03:07

impl Debug for OpfsConfig {
fn fmt(&self, _f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
panic!()
Copy link
Member

Choose a reason for hiding this comment

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

We can derive it instead of panic.


impl OpfsBuilder {}

impl Debug for OpfsBuilder {
Copy link
Member

Choose a reason for hiding this comment

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

The same.

type BlockingDeleter = ();

fn info(&self) -> Arc<AccessorInfo> {
todo!()
Copy link
Member

Choose a reason for hiding this comment

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

Let's provide a default AccessorInfo instead.

use crate::Error;
use crate::ErrorKind;

impl From<JsValue> for Error {
Copy link
Member

Choose a reason for hiding this comment

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

Hi, please implement parse_js_error instead to avoid leak this public API to users.

@@ -148,6 +148,9 @@ pub use obs::*;
mod onedrive;
pub use onedrive::*;

mod opfs;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we need to gate this under cfg(wasm32)?

@Xuanwo
Copy link
Member

Xuanwo commented Mar 17, 2025

Hi, @Eason0729, to get this PR merged, we will need:

  • Other CI not affected.
  • cargo clippy --features=services-opfs passed on this PR.
  • No panic / todo in it.

@Eason0729 Eason0729 marked this pull request as draft March 17, 2025 12:17
@Eason0729 Eason0729 marked this pull request as ready for review March 17, 2025 12:53
@Eason0729
Copy link
Contributor Author

@Xuanwo, thanks for the feedback. I've just fixed those issues so that:

  1. All CIs are not affected(pass).
  2. cargo clippy --features=services-opfs passes.
  3. No panic!() or todo!() remains in the code.

Please let me know if there's anything else needed!

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Other LGTM, thank you for this good start.

@Xuanwo Xuanwo changed the title origin private file system support feat: Add origin private file system scaffold Mar 18, 2025
@github-actions github-actions bot added the releases-note/feat The PR implements a new feature or has a title that begins with "feat" label Mar 18, 2025
Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thank you @Eason0729 for working on this!

@Xuanwo Xuanwo merged commit 11054d6 into apache:main Mar 18, 2025
243 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
releases-note/feat The PR implements a new feature or has a title that begins with "feat"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Origin Private File System support
2 participants