Support for window.fetch

[billiegoose]

I'm starting to work on issue #182 . It's very basic right now. , and I haven't tested to see if it works. I also can't get npm test to run on my machine so I'm relying on Travis for feedback.

TODOs

• [x] Configuration option preferXHR: Use XHR instead of Fetch
• [x] Test factory: Create fetch / non-fetch variants.
• [x] rename to HttpRequestFS?
• [x] update documentation

[billiegoose]

Oh what fun: your Travis is running on precise and mine is running on trusty - your Firefox 30 didn't have "fetch" and the tests failed, my Firefox 50 does have fetch but failed for some other reason. This is really great for testing!

[jvilk]

@wmhilton isn't it, though? 😄 That's why I have it all configured!

Why isn't npm test running? It should run on all major operating systems (Mac/Linux/Windows). That should be filed as a bug!

[jvilk]

I added some things to your TODOs. For the purpose of testing, we should have an option to explicitly disable fetch that defaults to whether or not fetch is available (e.g. !!global.fetch). You should modify the xhr_factory in the test folder to create variants with this option on and off.

That means the selection of which helper to use from xhr.ts will be decided by the file system, and not at initialization time like it is now. To avoid redundant conditionals (e.g. if (shouldFetch) all over the code), you can add private fields to the file system that save a reference to the chosen method of fetching / getting the size of files at file system construction time.

[billiegoose]

Newly refactored code:

• Adds a use option to the XmlHttpRequestOptions interface with values 'auto', 'xhr', and 'fetch'. ("use" feels a little too generic, but I couldn't think of a better one at the moment. requestType? underlyingTech?)
• Modifies isAvailable() so it will return true in environments with XmlHttpRequest OR fetch
• Adds a new private field _useFetch to remember selection
• Moved fetchFileAsync and fetchFileSizeAsync out of xhr.ts and into generics/fetch.ts

[billiegoose]

New changes:

• renames use to preferXHR and makes it a boolean
• modifies supportsSynch() to return false if XMLHttpRequest is unavailable
• moves the definitions for _requestFileAsync and _requestFileAsync to the constructor function
• fixes the error handling in fetch.ts

[jvilk]

(I added two more super picky comments that you don't have to address.)

[billiegoose]

I'm sure this is just my lack of familiarity with karma, but I can't figure out what/where/how to modify the tests to add a "preferXHR: false" variant. Any chance you could add that? Also, what do you think of the current code? I think I addressed all your feedback points, but you might still find ways to improve it.

[jvilk]

It's not in Karma! There are TypeScript files that create multiple variants of each file system for testing (hence, they are 'factories').

The following is the one for XHRFS, which currently has two variants (listing URL passed to Create() which is async, listing URL passed to constructor which is sync but will be removed in next major revision):

https://github.com/jvilk/BrowserFS/blob/master/test/harness/factories/xhrfs_factory.ts#L4

I'd like you to modify the test so that:

• The existing two XHR file systems prefer XHR, preserving the existing behavior.
• Add a third file system configuration that does not prefer XHR.

Make sense?

That's my last request!

[jvilk]

@wmhilton I just did the big 2.0 merge. I promise that's the last churn on this pull request!

This feature will go into the first 2.0 release.

Let me know if you've given up on the final few changes, and I will solve the merge conflicts and add the needed test changes.

[billiegoose]

Let me know if you've given up on the final few changes, and I will solve the merge conflicts and add the needed test changes.

Could you? I keep squinting at the code in xhrfs_factory.ts and I see how there are branches (the if statements) but I don't actually see how that turns into two test variants. Wait, unless... is the cb being used to append a test to the end of a hidden queue rather than being a continuation? 🤔

Um... I'm sorry @jvilk I just don't have enough experience for it to click. If you add the test, and then I look at the diff, I'm sure I'll slap my head and say "doh!" but right now I'd just be shooting in the dark.

[jvilk]

@wmhilton All merged! You can check out the changes I made if you want. I'll get on the other tasks before the 2.0 release (e.g. the rename, maintaining backwards compat, etc).