[BREAKING] (all platforms): remove "window.open" overwrite

[NiklasMerz]

Closes #599

Platforms affected

All

Motivation and Context

See #599

Testing

Tested shortly with https://github.com/dpa99c/cordova-plugin-inappbrowser-test

Checklist

• I've run the tests to see all new and existing tests pass
• I added automated test coverage as appropriate for this change -> Some tests got stuck a first but restarting CI fixed that
• Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
• If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
• I've updated the documentation if necessary

[brodycj]

This looks like a major, breaking change. I just added [MAJOR] prefix to the title to make this extra clear. I also added this PR to the 4.0.0 milestone, for the next major release of this plugin.

I would definitely favor this kind of an update, unfortunately not confident enough with this plugin to formally approve this proposal.

[NiklasMerz]

Yes this is a breaking change intended for the next major release. I raised #599 to discuss this and make a decision when time has come for a major release.

If this is released we could check and possibly close the issues around window.open.

[brodycj]

Thanks. I think I cannot stress enough how important it is to make it absolutely clear if a PR contains a breaking change. While we should expect current InAppBrowser maintainers to pick this up, we have seen maintainers come and go due to external factors. (I can only imagine if another maintainer would just do LGTM, merge and release without realizing that this is a breaking change ... kaboom!)

It was certainly not clear to me from first glance in this PR and #599 that you wanted to discuss the next major release in #599.

That said, I just sent a proposal to the mailing list to do a version bump and get this PR reviewed & potentially merged. Thanks for your quick work on this.

[rpanadero]

I agree with @NiklasMerz, this change will fix many issues related to window.open overwrite side effects. I'm looking forward to seeing this PR merged in order to remove the hack I have in my application. Thanks.

[brodycj]

Maintainers are a bit overloaded. I would highly recommend that you follow up with us on the mailing list or on Slack, please follow the links in the footer of cordova.io or cordova.apache.org.

[NiklasMerz]

Next major version is work in progress right now. We can merge this once we get some review for this.

We should not forget to mention this in the changelog and release blog post, since this is a breaking change. I changed the title to make that the visible in the commit message.

[timbru31]

LGTM. I always felt that a core plugin shouldn’t overwrite a function such as window.open.

[NiklasMerz]

Thanks @timbru31 and @erisu. Whenever 4.0.0 is prepared this should be merged.

[NiklasMerz]

@erisu Does this cover electron, too? Does electron use browser?

[timbru31]

What's the status on this, Niklas? Could you solve the electron question?
(AFAIK electron uses electron platform and browser as a fallback).

Given the fact that the master already contains breaking changes, we could merge this.

[NiklasMerz]

I think it's good to merge. As far as I can remember this should not affect electron since it has not implementation for IAB.

[erisu]

It is OK in regards to Electron.