serhiikh-gk commented on PR #1094: URL: https://github.com/apache/cordova-plugin-inappbrowser/pull/1094#issuecomment-3657018156
> 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
