Skip to content

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.

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