Generic filehandle remoteFile refactor

[garrettjstevens]

These are a couple proposed changes to help me get cors-busting working in jb-desktop. Basically they are:

• Add stat method to the GenericFilehandle interface
• Refactor RemoteFile so all the fetching happens in a getFetch method
• Try to parse the content-length header for file size if it's not a range request
• Some various TypeScript tweaks

Basically I'm extending RemoteFile in jb-desktop so that the getFetch method can fall back to using node-fetch if fetch fails (which may be due to cors).

[codecov]

Codecov Report

Merging #42 into master will increase coverage by 0.64%.
The diff coverage is 92.59%.

@@            Coverage Diff             @@
##           master      #42      +/-   ##
==========================================
+ Coverage   89.28%   89.93%   +0.64%     
==========================================
  Files           4        4              
  Lines         140      149       +9     
  Branches       49       54       +5     
==========================================
+ Hits          125      134       +9     
  Misses         15       15

Impacted Files	Coverage Δ
src/blobFile.ts	78.72% <ø> (ø)	⬆️
src/localFile.ts	90.47% <0%> (ø)	⬆️
src/remoteFile.ts	95.89% <100%> (+0.57%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 04ea4ab...eaa3749. Read the comment docs.

[rbuels]

these look great to me, would like to see what @cmdcolin thinks before merging though

[cmdcolin]

This is pretty good code wise but I am not a big fan of our library code depending on stat. I know it already exists in some libraries but it complicates any CORS setup by requiring additional headers and such, and it would now require allowing HEAD request CORS setup too. We can just say "whatever screw CORS we'll proxy it/use node fetch" but not hugely into this either.
It's true that web servers can legally return a 416 for beyond the file size requests https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/416 but my experience is that this does not happen from most webservers, it just returns a shorter response. Is there something that renewed focus on stat?

[garrettjstevens]

Weird, I wasn't getting notifications for this PR. So I added some additional stuff to stat because I needed it in the first approach I took at cors-busting, but I ended up doing something else. The stat additions are still in here, but I don't actually depend on them in my jb-desktop code. I changed the name of the PR to better reflect what's actually important.

[rbuels]

Looks to me like this adds a pretty darn robust stat implementation, but doesn’t add any overhead to code that doesn’t want to use it. Is that what you guys are getting from this?

[garrettjstevens]

@cmdcolin any other concerns or questions with merging this? I want to make sure I actually addressed what you were asking about.

[cmdcolin]

I had concerns including
-why is this change to use head request bring introduced
I'm also concerned about the usage of get fetch function being in this library... It seems like the wrong abstraction

[garrettjstevens]

The HEAD request is basically a way to increase the robustness of the stat method. It tries to use the HEAD request's "content-length" to determine the file size, and if that doesn't work it falls back to using a range request and "content-range". It's only used in the stat method, and then only if the file size hasn't already been determined.

As for the getFetch method, it's just refactoring the fetching logic that was already there into a single method. I thought it would be good for a class extending RemoteFile to be able to customize the fetching logic (it worked out well for what I wanted to do). RemoteFile already allows a custom fetch to be provided, so this method made sense to be able to add the ability to do special things with that custom fetch. That being said, I probably need to make it a protected method instead of a public one.

What about the abstraction do you have concerns with?

[garrettjstevens]

Closing in favor of #43.