Multi instance support

[serhiikh-gk]

Platforms affected

ios, android

Motivation and Context

• standard implementation of plugin does not support multiple instances of browsers
• possible to close hidden browser
• hide event is sent when browser is hidden

Description

Add multi-instance support:

• multiple browser instances support for ios;
• multiple browser instances support for android;
• fix an issue when browser is not closed if hidden (ios);
• send hide event;
• add unit tests with jest;
• add types checks with tsd;
• update package information;

Testing

• I have demo application where I can test all standard api (https://github.com/serhiikh-gk/cordova-plugin-inappbrowser-demo)
• Add unit tests with jest

[breautek]

My review is limited to the meta details on the repository, rather than a code review.

Overview is that version or engine details shouldn't be changing as part of this PR. If there are breaking changes that warrants a 7.x then the breaking changes should be noted in this PR description and updating the vesion to 7.0.0-dev should be done with it's own standalone PR. This is to protect ourselves from merging in other breaking changes and then having to rollback this PR for whatever reason, the major version and engine bumps will stay.

For unit testing, Cordova uses paramedic tool that is responsible for running the tests in a cordova webview environment. So introducing jest probably won't fly. Internally paramedic uses jasmine test runner so any test written may need to be refactored.

Introducing tsd may be ok as it expands coverage against something we historically don't really ever test.

I didn't spend much time looking at the actual code, but I'll note that the plugin already supports holding different IAB references and communicating with those references and by extension should already support maintaining multiple instances of IAB windows. Introducing an entire new InAppBrowserMulti API is a bit of a red flag for me.

If something is not working as expected there, perhaps that warrants a discussion.

[breautek]

unit tests should be ran via cordova's paramedic tool which uses jasmine behind-the-scenes. See the existing tests.

Ref: https://github.com/apache/cordova-paramedic

[serhiikh-gk]

I've never worked with that, but I'll try to figure out if make sense to keep new tests or remove. Thank you for the hint.

[serhiikh-gk]

My review is limited to the meta details on the repository, rather than a code review.

Overview is that version or engine details shouldn't be changing as part of this PR. If there are breaking changes that warrants a 7.x then the breaking changes should be noted in this PR description and updating the vesion to 7.0.0-dev should be done with it's own standalone PR. This is to protect ourselves from merging in other breaking changes and then having to rollback this PR for whatever reason, the major version and engine bumps will stay.

For unit testing, Cordova uses paramedic tool that is responsible for running the tests in a cordova webview environment. So introducing jest probably won't fly. Internally paramedic uses jasmine test runner so any test written may need to be refactored.

Introducing tsd may be ok as it expands coverage against something we historically don't really ever test.

I didn't spend much time looking at the actual code, but I'll note that the plugin already supports holding different IAB references and communicating with those references and by extension should already support maintaining multiple instances of IAB windows. Introducing an entire new InAppBrowserMulti API is a bit of a red flag for me.

If something is not working as expected there, perhaps that warrants a discussion.

• regarding unit tests: these tests are testing only js part, but not a plugin itself. Existing tests should still work. I do not have much knowledge about cordova-paramedic but I think it is kind of testing of real cordova app with real plugin. But I'll check first and return back with more info.
• regarding InAppBrowserMulti: idea was to keep InAppBrowser for compatibility reason and add InAppBrowserMulti. but I agree that it can be merged together. Are we talking about InAppBrowserMulti.js or also about new InAppBrowserMulti in objective-c and java code? The problem ii with current plugin, that does not matter how many time you use open api, it will display you the same content in browser, it keeps only last instance alive/active and you can't communicate anymore with older instances. I'll prepare some demo with debug information to better visualise the problem.

[breautek]

it keeps only last instance alive/active and you can't communicate anymore with older instances

If that's the case, then we can probably update the native side to retain references of IAB instances, and assign them IDs so that we can map calls back onto them.

[serhiikh-gk]

it keeps only last instance alive/active and you can't communicate anymore with older instances

If that's the case, then we can probably update the native side to retain references of IAB instances, and assign them IDs so that we can map calls back onto them.

basically it is what was done with CDVInAppBrowserWindowManager (ios) / InAppBrowserWindowManager (android). To hold the instances, assign unique windowId, and send events to correct browser instance.

Seems like there were also issues reported + some discussions on stackoverflow:
#533
#617
#752
#937

Let me first harmonise InAppBrowser and InAppBrowserMulti or prove that they should stay separate + make unit test to work via cordova-paramedic or remove them. And I'll back to MR.