Closed billiegoose closed 7 years ago
Good to hear from you, @wmhilton! I've had my eye on the fetch API since it supports streaming downloads in some browsers, but I have not had time to tackle adding it to BrowserFS yet.
I actually wouldn't mind renaming the XmlHttpRequest
file system to something more generic, and have it use fetch
when available. Maybe HTTPDownloadFS
or something like that? Then, we can refactor xhr.ts
to be more generic and use fetch
when available.
I'm happy to deputize all of this to you, and review your PR when you are ready. :)
(Oh, and for testing purposes, you might want to add an extra option to the file system that disables the use of fetch
. Then, modify the xhrfs_factory
in test/factories
to construct variants with and without fetch.)
We could rename it and have the old name still available for backwards compatibility. I hate needlessly breaking things by changing names. I wouldn't name it HTTPDownloadFS though because I imagine it would also work for HTTPS and HTTP/2. Maybe just Download FS.
But yeah, at least at first glance it seems like the easiest approach is to extend xhr.ts with feature detection for fetch and use that if XmlHttpRequest isn't available.
@wmhilton Yeah, that's what I meant. I'm trying to follow semver -- adding warning messages to things that will be deprecated in minor releases, and then removing them in major releases. Right now, I'm working on #176 that does this for some of the more messy initialization routines.
In this case, I'd make BrowserFS.FileSystem.XmlHttpRequest
a getter
that returns Download FS and emits a console.warn
message about future deprecation.
We can think more on an appropriate name. It is a download-based file system using some form of the hypertext transport protocol, which is why I suggested HTTPDownloadFS. I'm fine with moving forward with DownloadFS
just to get the ball rolling.
What about just taking the Xml off and making it HttpRequestFS? Although frankly, I've just been rolling with keeping the old name for now and just adding support for fetch
based operations in xhr.js
.
Or would it just be HttpRequest? (I'm not clear when to add an FS suffix and when not.)
Or would it just be HttpRequest? (I'm not clear when to add an FS suffix and when not.)
Yeah, it's not consistent right now, and grew organically as I added backends.
HttpRequestFS
is OK with me! You can merge with the old name, and I can add in the name fix + deprecation notice.
Do we want to pull out the fetch
versions in a separate file from the XmlHttpRequest
versions? I feel like having them in xhr.ts
is a little anachronistic.
@wmhilton Yeah. See my new comment in the PR.
Merged and will be released for 2.0.
Same idea as the
XmlHttpRequest
but using the newerfetch
API. I just discovered thatXmlHttpRequest
is not available in the Service Worker scope. (I am having success using IndexedDB FS in my Service Worker though.)Notes:
XmlHttpRequest.ts
already has all the actual XmlHttpRequests nicely factored into../generic/xhr.ts
. Of that interface, it looks like the only functions that need to be implemented areasyncDownloadFile
andgetFileSizeAsync
.Any suggestions with how to proceed? (e.g. is there a better way to start than copy & paste XmlHttpRequest?) I'm willing to do the work and make a PR.