[file system][ios] Add download and upload in background

[lukmccall]

Why

Part of #5841.

How

• makes background session
• creates task delegate
• uses NSURLSessionUploadTask and NSURLSessionDownloadTask

ToDo

• pod install in bare expo

Test Plan

• https://snack.expo.io/@lukaszkosmaty/expo-file-system---dowanload-upload-in-background ✅
• NCL ✅

[tsapeta]

Generally looks good 👍

• Changelog entry and docs are missing 😞
• Would it be possible to add examples in NCL and add some tests?

Also some comments below 👇

[lukmccall]

@tsapeta, I'll add an entry to the changelog and update docs in separate PR.

It'll be difficult to add tests - we need a server to test upload functionality properly. However, I can add a session type switch to NCL.

[bbarthec]

Great job! 💯
Is it possible for you to create some python or node script that would allow us to test it? 😅
It would be nice to be sure it always works 🤔

[tsapeta]

@tsapeta, I'll add an entry to the changelog and update docs in separate PR.

Sorry but I'm against adding changelog entries in different PRs - changelogs must always be updated together with a change that changed anything in module's behavior. We're going to use Danger.js to enforce changelog entries on all PRs so why should we make any exceptions before the actual Danger.js action is done? Let's say that before this happens, I'm the one who manually checks all these PRs 😃

[sjchmiela]

Nice work! 👍

[sjchmiela]

Sorry it took me so long! Thank you for making the structure clear! Looking forward to reviewing it again!

[sjchmiela]

The conversation at #7380 (comment) has been resolved — was it by accident, or is it a strong decision to keep using enums for HTTP methods, or have I missed some discussion?

[sjchmiela]

Nonetheless, I think this enum should be named with a singular form, i.e. FileSystemHttpMethod.

[lukmccall]

Sorry, it was resolved by accident.
However, I think we should give the user the ability to change the HTTP method. And we can do it by using strings or enum. I prefer using enum - it is safer. If you strongly disagree with this, I can change it ;)

[bbarthec]

LGTM 💯 I think we are ready to ship it once all concerns are resolved 😄

[sjchmiela]

🏅

[sjchmiela]

🙇

[sjchmiela]

I think this is a fourth time I mention this concern, it somehow always gets discarded on push 😃 — how about accepting a string httpMethod, like fetch does?

[lukmccall]

I've commented on that here: #7380 (comment) ;)

Moreover, I don't like strings here, cause someone might pass get - that will crash the app. So, it would be nice to have some kind of validations. And what if someone misspelled it.

[sjchmiela]

IMHO it would be best to:

• try-catch not to crash the app if a user passed a get
• throw an error with a helpful message if an invalid method has been passed in.

This would make sure we don't limit the developers if they wanted to use some other HTTP method that is supported we don't know of. But I don't have a very strong opinion, so I will defer to @bbarthec's opinion on that.

[bbarthec]

@lukmccall, I've discussed the thing with @sjchmiela and I think that going with fetch-like approach is desirable: https://fetch.spec.whatwg.org/#methods
I suggest enforcing TS type: type AcceptedHttpMethod = 'POST' | 'PUT' | 'PATCH' is sufficient guarding point, but later on let's go with String.toUpperCase() anyway while passing arguments to native side.
Are there any consequences of passing method that is not expected, eg. CHICKEN (I mean: are we semantically base on this value?)

[lukmccall]

Are there any consequences of passing method that is not expected, eg. CHICKEN (I mean: are we semantically base on this value?)

The task will be canceled by the system later on. Then we will reject the promise. So, I think that everything will work fine ;)

[sjchmiela]

So… since first we deactivate the dispatcher and only then invalidateAndCancel the sessions in EXFileSystem's dealloc, doesn't it mean that here we wouldn't act on tasks cancellation?

Is it the way we want to handle this? Don't we want to reject the Promise, or inform of the termination?

This _isActive logic seems too easy 😃.

[lukmccall]

Yes, we deactivate the dispatcher before invalidateAndCancel because we don't want to act on the task events.
We are in the middle of the deallocating of the react context. So, I believe we shouldn't reject the Promise or inform of the termination. Tasks will be canceled, but we don't know when.

[sjchmiela]

Do you think that adding a separate invalidate call to native modules would make it easier to handle this situation? If we invalidatedAndCancelled sessions while invalidating modules maybe the task:didCompleteWithError: would be called in time before deallocating the bridge and a Promise rejection would come through? If this would be the case I think it could be worth to implement it… 🤔

[sjchmiela]

IMHO it would be best to:

• try-catch not to crash the app if a user passed a get
• throw an error with a helpful message if an invalid method has been passed in.

This would make sure we don't limit the developers if they wanted to use some other HTTP method that is supported we don't know of. But I don't have a very strong opinion, so I will defer to @bbarthec's opinion on that.

[sjchmiela]

Do you think that adding a separate invalidate call to native modules would make it easier to handle this situation? If we invalidatedAndCancelled sessions while invalidating modules maybe the task:didCompleteWithError: would be called in time before deallocating the bridge and a Promise rejection would come through? If this would be the case I think it could be worth to implement it… 🤔

[lukmccall]

Do you think that adding a separate invalidate call to native modules would make it easier to handle this situation? If we invalidatedAndCancelled sessions while invalidating modules maybe the task:didCompleteWithError: would be called in time before deallocating the bridge and a Promise rejection would come through? If this would be the case I think it could be worth to implement it… 🤔

I think, this a good idea in general. However, I'm not sure if this helps here. Maybe we can reject all promises before the react context dies, but I don't think we need this in that particular case. Moreover, the js code will not know what causes rejection. So, the user could write a code that retries the download and this will cause bugs - the file-system module will no longer exist.

[sjchmiela]

I think, this a good idea in general. However, I'm not sure if this helps here. Maybe we can reject all promises before the react context dies, but I don't think we need this in that particular case. Moreover, the js code will not know what causes rejection. So, the user could write a code that retries the download and this will cause bugs - the file-system module will no longer exist.

So, to sum up, the only updates we drop are download/upload progress updates which happened after deallocing EXFileSystem? So having no update on such request is like communicating "this request didn't complete", so the user won't be tempted to mark the request as either done or failed?

What I'm worried about is the scenario when a developer manages the downloads depending on the callbacks, so eg.:

• the user requests to download eg. a map of Poland
• developer saves information about a download-in-progress to AsyncStorage (that it is being downloaded)
• the app is closed (refreshed, whatever, EXFileSystem is dealloced)
• the app is opened
• the information that the developer saved (that the download is processing) is invalid anymore

Is this how one would use the FileSystem module? Or is it obvious to always check if the download is processing when the app starts? How? 🤔