You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cordova.apache.org by GitBox <gi...@apache.org> on 2019/12/30 23:45:41 UTC
[GitHub] [cordova-plugin-inappbrowser] NiklasMerz opened a new pull request
#600: (all platforms): remove "window.open" overwrite
NiklasMerz opened a new pull request #600: (all platforms): remove "window.open" overwrite
URL: https://github.com/apache/cordova-plugin-inappbrowser/pull/600
Closes #599
<!--
Please make sure the checklist boxes are all checked before submitting the PR. The checklist is intended as a quick reference, for complete details please see our Contributor Guidelines:
http://cordova.apache.org/contribute/contribute_guidelines.html
Thanks!
-->
### Platforms affected
All
### Motivation and Context
See #599
### Testing
<!-- Please describe in detail how you tested your changes. -->
Tested shortly with https://github.com/dpa99c/cordova-plugin-inappbrowser-test
### Checklist
- [x] I've run the tests to see all new and existing tests pass
- [ ] I added automated test coverage as appropriate for this change
- [x] Commit is prefixed with `(platform)` if this change only applies to one platform (e.g. `(android)`)
- [x] 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](https://help.github.com/articles/closing-issues-using-keywords/))
- [x] I've updated the documentation if necessary
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-plugin-inappbrowser] brodybits commented on issue #600:
[MAJOR] (all platforms): remove "window.open" overwrite
Posted by GitBox <gi...@apache.org>.
brodybits commented on issue #600: [MAJOR] (all platforms): remove "window.open" overwrite
URL: https://github.com/apache/cordova-plugin-inappbrowser/pull/600#issuecomment-569844633
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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-plugin-inappbrowser] NiklasMerz edited a comment on issue
#600: [BREAKING] (all platforms): remove "window.open" overwrite
Posted by GitBox <gi...@apache.org>.
NiklasMerz edited a comment on issue #600: [BREAKING] (all platforms): remove "window.open" overwrite
URL: https://github.com/apache/cordova-plugin-inappbrowser/pull/600#issuecomment-613432346
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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-plugin-inappbrowser] NiklasMerz merged pull request #600:
[BREAKING] (all platforms): remove "window.open" overwrite
Posted by GitBox <gi...@apache.org>.
NiklasMerz merged pull request #600: [BREAKING] (all platforms): remove "window.open" overwrite
URL: https://github.com/apache/cordova-plugin-inappbrowser/pull/600
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-plugin-inappbrowser] timbru31 commented on issue #600:
[BREAKING] (all platforms): remove "window.open" overwrite
Posted by GitBox <gi...@apache.org>.
timbru31 commented on issue #600: [BREAKING] (all platforms): remove "window.open" overwrite
URL: https://github.com/apache/cordova-plugin-inappbrowser/pull/600#issuecomment-613410989
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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-plugin-inappbrowser] brodybits commented on issue #600:
[MAJOR] (all platforms): remove "window.open" overwrite
Posted by GitBox <gi...@apache.org>.
brodybits commented on issue #600: [MAJOR] (all platforms): remove "window.open" overwrite
URL: https://github.com/apache/cordova-plugin-inappbrowser/pull/600#issuecomment-581925032
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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-plugin-inappbrowser] rpanadero commented on issue #600:
[MAJOR] (all platforms): remove "window.open" overwrite
Posted by GitBox <gi...@apache.org>.
rpanadero commented on issue #600: [MAJOR] (all platforms): remove "window.open" overwrite
URL: https://github.com/apache/cordova-plugin-inappbrowser/pull/600#issuecomment-581802157
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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-plugin-inappbrowser] NiklasMerz commented on issue #600:
[BREAKING] (all platforms): remove "window.open" overwrite
Posted by GitBox <gi...@apache.org>.
NiklasMerz commented on issue #600: [BREAKING] (all platforms): remove "window.open" overwrite
URL: https://github.com/apache/cordova-plugin-inappbrowser/pull/600#issuecomment-613432346
I think it's good to merge. As far as I can remember this should not affect electron.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-plugin-inappbrowser] NiklasMerz commented on issue #600:
[BREAKING] (all platforms): remove "window.open" overwrite
Posted by GitBox <gi...@apache.org>.
NiklasMerz commented on issue #600: [BREAKING] (all platforms): remove "window.open" overwrite
URL: https://github.com/apache/cordova-plugin-inappbrowser/pull/600#issuecomment-582259268
Thanks @timbru31 and @erisu. Whenever 4.0.0 is prepared this should be merged.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-plugin-inappbrowser] erisu commented on issue #600:
[BREAKING] (all platforms): remove "window.open" overwrite
Posted by GitBox <gi...@apache.org>.
erisu commented on issue #600: [BREAKING] (all platforms): remove "window.open" overwrite
URL: https://github.com/apache/cordova-plugin-inappbrowser/pull/600#issuecomment-613440951
It is OK in regards to Electron.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-plugin-inappbrowser] NiklasMerz commented on issue #600:
[MAJOR] (all platforms): remove "window.open" overwrite
Posted by GitBox <gi...@apache.org>.
NiklasMerz commented on issue #600: [MAJOR] (all platforms): remove "window.open" overwrite
URL: https://github.com/apache/cordova-plugin-inappbrowser/pull/600#issuecomment-569831966
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`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-plugin-inappbrowser] NiklasMerz commented on issue #600:
[BREAKING] (all platforms): remove "window.open" overwrite
Posted by GitBox <gi...@apache.org>.
NiklasMerz commented on issue #600: [BREAKING] (all platforms): remove "window.open" overwrite
URL: https://github.com/apache/cordova-plugin-inappbrowser/pull/600#issuecomment-581933342
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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-plugin-inappbrowser] NiklasMerz commented on issue #600:
[BREAKING] (all platforms): remove "window.open" overwrite
Posted by GitBox <gi...@apache.org>.
NiklasMerz commented on issue #600: [BREAKING] (all platforms): remove "window.open" overwrite
URL: https://github.com/apache/cordova-plugin-inappbrowser/pull/600#issuecomment-591816163
@erisu Does this cover electron, too? Does electron use browser?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org