-
Notifications
You must be signed in to change notification settings - Fork 596
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
Conversation
Thank you for your review(and I missing it earlier). I will address your suggestions tomorrow. |
core/src/services/opfs/backend.rs
Outdated
|
||
impl Debug for OpfsConfig { | ||
fn fmt(&self, _f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
panic!() |
There was a problem hiding this comment.
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
.
core/src/services/opfs/backend.rs
Outdated
|
||
impl OpfsBuilder {} | ||
|
||
impl Debug for OpfsBuilder { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same.
core/src/services/opfs/backend.rs
Outdated
type BlockingDeleter = (); | ||
|
||
fn info(&self) -> Arc<AccessorInfo> { | ||
todo!() |
There was a problem hiding this comment.
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.
core/src/services/opfs/error.rs
Outdated
use crate::Error; | ||
use crate::ErrorKind; | ||
|
||
impl From<JsValue> for Error { |
There was a problem hiding this comment.
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.
core/src/services/mod.rs
Outdated
@@ -148,6 +148,9 @@ pub use obs::*; | |||
mod onedrive; | |||
pub use onedrive::*; | |||
|
|||
mod opfs; |
There was a problem hiding this comment.
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)
?
Hi, @Eason0729, to get this PR merged, we will need:
|
@Xuanwo, thanks for the feedback. I've just fixed those issues so that:
Please let me know if there's anything else needed! |
There was a problem hiding this 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.
There was a problem hiding this 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!
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?