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

feat: add opfs storage adapter #294

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

openscript
Copy link
Contributor

This adds a minimal implementation of a OPFS storage adapter.

@openscript openscript mentioned this pull request Feb 21, 2024
@pvh
Copy link
Member

pvh commented Feb 21, 2024

Thanks, @openscript! I'll give it a proper review soon and let you know if I have any requests before merging. I don't suppose you've benched this against IndexedDB? I was thinking this morning I'm not sure which one to recommend to folks.

@openscript
Copy link
Contributor Author

No I've never really benched it, not in the context of automerge but also not outside of that. Just before I was looking for a benchmark, but I didn't really find a general and current browser storage benchmark. Maybe a project for a rainy weekend :)

@pvh
Copy link
Member

pvh commented Apr 4, 2024

Hi! I'm catching up on PRs and spent a bit of time in here. I don't think it's ready to merge as-is. I'd like to make a few suggestions for improvement (which if time allows in the future I may take on myself.)

First: I suspect that passing in the root directory handle instead of sourcing it from the window directly would make this adapter compatible with the Chrome File System APIs as well -- this is quite a cool concept because it could allow sharing of a downloaded storage pool with processes running outside the browser.

That said, if that were going to work you'd need to align your file location / key mapping system with the nodefs implementation. This is probably a good idea anyway, as the current implementation will suffer quite badly from the cost of scanning large filesystem directories if the user has a heap of data.

I'm going to mark this as a draft for now but I suspect we should be able to show that this would be a better default implementation for the project than the nodefs implementation. In fact, it might be easier to port that over... hmmm...

@pvh pvh marked this pull request as draft April 4, 2024 20:47
@pvh pvh self-requested a review April 4, 2024 20:47
@pvh pvh self-assigned this Apr 4, 2024
@pvh
Copy link
Member

pvh commented Jul 24, 2024

Had a close look to consider landing this as part of the next release but I realized it's putting all the files into one flat directory. We have had reports that this approach leads to serious performance problems for large collections. In fact, it's the same reason that both git itself and the NodeFS implementation fragment the namespace by prefix.

Fixing this wouldn't be too hard -- we can probably crib from the NodeFS implementation -- but I don't think it's something I can sneak in at this hour of my work day. Are you still using it, @openscript? Any experience reports after a few months?

@pvh pvh force-pushed the main branch 2 times, most recently from e61f8e3 to d3d1a7d Compare July 26, 2024 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants