search

riboseinc / isogit-lfs

LFS helpers on top of Isomorphic Git
MIT License
7 stars 3 forks source link

Fork with browser support #5

Open catdevnull opened 1 year ago

catdevnull commented 1 year ago

Hi, I made a fork with hacky browser support. There are some rough edges and it's not fully tested, but it seems to work. https://gitea.nulo.in/Nulo/isogit-lfs

ronaldtse commented 1 year ago

Thank you @catdevnull !

@ribose-jeffreylau could you see if we can port back the changes via a PR? Thanks!

strogonoff commented 1 year ago

@catdevnull I completely failed to notice this as I did a refactor that aims to remove the Node dependency. My refactor happened independently of yours but I’m happy to see the similarities in our approaches.

Like you, I decided to adopt the same approach as in isomorphic-git of accepting an fs parameter, though like IsoGit I accept FsClient that can be either callback- or promise-based. So far I decided against introducing a new dependency on path-browserify for path manipulation, not sure yet if the overhead would be worth it. Similar with buffers; instead of adding a polyfill I’ve decided to instead switch to Uint8Arrays, since those work both in Node and in the browser natively it is perhaps a more proper approach—though we will see if there are performance implications.

Can probably expect this library to be as isomorphic as isomorphic-git in version 0.2.0 planned to be released later this month after some testing.

strogonoff commented 1 year ago

There may be a breaking change in 0.2.0 as I want the lower-level (but nevertheless publicly exported) utility readPointer() to accept directly gitdir and not require a dir—the current approach of accepting working directory may not work with bare repositories, and generally readPointer() really only works with Git repository itself and doesn’t care about working directory at all.

catdevnull commented 1 year ago

So far I decided against introducing a new dependency on path-browserify for path manipulation, not sure yet if the overhead would be worth it. Similar with buffers; instead of adding a polyfill I’ve decided to instead switch to Uint8Arrays, since those work both in Node and in the browser natively it is perhaps a more proper approach—though we will see if there are performance implications.

it's most certainly "the right thing to do"; i just wanted to get it with the least amount of effort working enough for my use case in which bundle size doesn't matter. considering that the buffer package uses Uint8Array internally i'm guessing it's not going to worsen performance.

catdevnull commented 1 year ago

@catdevnull I completely failed to notice this as I did a refactor that aims to remove the Node dependency. My refactor happened independently of yours but I’m happy to see the similarities in our approaches.

:') yes, also, my work might not be very PR-able because i changed a lot of the tooling and indentation around.

strogonoff commented 1 year ago

@catdevnull can I ask whether you access the readPointer() function directly? I don’t think this library has many users, so if you’re one I was wondering whether a signature change will break anything for you.

catdevnull commented 1 year ago

nope, i don't use readPointer as my usecase only writes to the repo. i don't mind signature changes though, go ahead!