Synchronized tables, separated requests and network logic and other...

[chrusart]

COPY OF: wkh237#636

Hi,

this PR came from crashes we still get (some description):
wcandillon/react-native-img-cache#95

...but evolved.

Whole RNFetchBlobNetwork class actually do two things:

• keeping tasks references to cancel them, change their progress config etc.,
• session and requests logic.

I moved session and requests logic into separate file, now it's more clean what do what.
NSMapTable seemed to be used incorrectly or just it's (weak) features not used (it seems from log NSLog(@"object released by ARC."); that it should or was used), anyway, it's now instantiated with NSMapTableWeakMemory option for objects, so requests will be released, default in NSMapTable is to use strong references.

sharedInstance is used instead tables and dictionaries instances created on load. This pattern is used all around but please give me a feedback why previous is better solution, maybe I missed something.

Other things done in this PR when implementing:

• checkExpiredSessions didn’t find anywhere, removed from header
• __block storage type not needed, variables not used in blocks in scope
• extern storage type not used in .m files
• nullability type in RNFetchBlobNetwork, should be checked and applied for method arguments in other classes
• unimplemented + (void) enableProgressReport:(NSString *) taskId, + (void) enableUploadProgress:(NSString *) taskId, - (void) sendRequest method declarations removed
  unused fileTaskCompletionHandler and dataTaskCompletionHandler properties and synthesizers removed
• other small warning messages fixes
• every single request is created on new RNFetchBlobNetwork instance so taskQueue.maxConcurrentOperationCount seems not work as expected -> made taskQueue instantiated per RNFetchBlobNetwork instance.

I can't find any place where actually something is added to expirationTable.

Does HTTPMaximumConnectionsPerHost = 10 have any impact as new session is created for every request (except defaultSessionConfiguration)? In docs it says that it applies to all sessions '...based on this configuration', what actually this means, the same instance, identifier or some comparison???

I'm aware of few fixes that are in 0.10.9 not included here.

TODO:

• subclass RCTEventEmitter to send events
• expired task logic?

This works for now for us, but didn't test everything and any feedback or testing would be great.

POST 16.01.2018

What about applying:
http://google.github.io/styleguide/objcguide.html
I agree with that code guideline mostly.

POST 19.01.2018:

https://github.com/flatfox-ag/react-native-fetch-blob/tree/exception_fixes 2 days already in production, no exceptions yet.

This branch have all these fixes:
wkh237#619
wkh237#499

and all changes from:
https://github.com/flatfox-ag/react-native-fetch-blob/tree/synchronized

feel free to:
"react-native-fetch-blob": "github:flatfox-ag/react-native-fetch-blob#a46bf8180d76131eafe84ab44e0436322073820c",

As we use it in production I removed WIP note.
Stuff in TODO will come in different PRs I guess.

POST 25.01.2018

Week without crashes on production, fixes confirmed :)

[chrusart]

For now no crashes from this branch and from libraries that use fetch-blob on our production.

[chrusart]

Got some after merge errors with promises, working on that

[chrusart]

Looks good now, but HEAD not compatible anymore with 0.10.8 because interfaces changed in 0.10.9 from callbacks to promises and if someone used our branch to fix exceptions then use:
"react-native-fetch-blob": "github:flatfox-ag/react-native-fetch-blob#a46bf8180d76131eafe84ab44e0436322073820c", to still work with other libraries that use v0.10.8 (like react-native-cached-image)

P.S. anyway, always use commit hash instead branch name, cause it can be changed by owner.

[Traviskn]

@chrusart Thank you for this PR, it looks like you've put a lot of work in and you've even tested that it resolved issues for you in production! Am I correct in assuming that these are mostly internal changes and that there aren't any changes in the public API of the library with this PR?

[chrusart]

Hi @Traviskn , the 0.10.9 branch have changes that uses Promises instead callbacks, and that was done before, not by me. The problem is when somebody will use 'custom' version (branch) in his projects package.json. In that case native compiled code will be taken from this one, but if library has different version (react-native-cached-image for example has 0.10.8) then packager will download this version specially for this library and it will be in node_modules of this library. What happens then is that library will use his own react-native-fetch-blob javascript code which uses callbacks in 0.10.8, but native compiled code will be from version you have in your projects package.json.

So, for example mkdir in fs.js in 0.10.9:
return RNFetchBlob.mkdir(path)
and in master:

RNFetchBlob.mkdir(path, (err, res) => {
      if(err)
        reject(new Error(err))
      else
        resolve()
    })

(other: ls, createFile)

and it will crash the app if native code uses promises, but js callbacks.
This is something you have to carry about when doing some packager 'hack', mixing two versions together.

And coming back to your question about interface.
Public interface didn't change from what I see, it's just mixing two versions together is a problem.
EDITED (after not reading cached-image docs carefully):
If react-native-cached-image will change fetch-blob version from 0.10.8 to 0.10.9 then everything should be fine.
We use fetch-blob in project so we have it anyway in project package.json and we have to carry what version we use if other libraries use it as well.

I hope it's clear enough and I understood and tested correctly to get to this conclusions.

[Traviskn]

@chrusart Thanks for the overview!