-
Notifications
You must be signed in to change notification settings - Fork 112
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move all WordPressAuthenticator sources into this repo #14779
base: trunk
Are you sure you want to change the base?
Conversation
Generated by 🚫 Danger |
ef7a776
to
c4e4f34
Compare
📲 You can test the changes from this Pull Request in WooCommerce iOS by scanning the QR code below to install the corresponding build.
|
88a55cf
to
662b543
Compare
f161a2d
to
ca60ec7
Compare
ad9a8cc
to
05510e9
Compare
@@ -37,7 +37,11 @@ def tracks | |||
end | |||
|
|||
def wordpress_shared | |||
pod 'WordPressShared', '~> 2.1' | |||
pod 'WordPressShared', '~> 2.1-beta' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -1,4 +1,4 @@ | |||
IPHONEOS_DEPLOYMENT_TARGET = 15.0 | |||
IPHONEOS_DEPLOYMENT_TARGET = 16.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already set in the project but not update here and ignored because some target overrode it instead of inheriting (by deleting the entry at the target level)
Hey @mokagio, when can we expect this PR to be ready for review and merged? We have a project that touches on this code (pe5sF9-3mV-p2), and we are delaying it until this PR is merged to avoid any more unwanted conflicts. |
It got turned on by the 16.2 update and resulted in build failures.
This reverts commit d89e961.
6bc7e88
to
56e20a4
Compare
@hichamboushaba sorry you had to write that. Bad timing on my end... I had planned and started to work on this today p1737328507842779-slack, but something else came up, p1737344967048199-slack, so I only got to this now. Anyway, this is now ready to review and I hope it'll get looked at soon. Feel free to leave your review, too, much appreciated. |
hi @mokagio 👋 have you considered using |
Description
Removes the
WordPressAuthenticator-iOS
andWordPressKit-iOS
dependencies by folding both into the codebase. See rationale in #14329.I implemented this using one framework target in the WooCommerce project, which is then linked in the WooCommerce iOS app. I had initially tried having the code in a dedicated project, like the other frameworks like Yosemite, Storage, Networking, etc. do, but I run into some compilation issues and decided to move to the target setup. For what is worth, that is the same approach that WordPress and Jetpack use.
We could consider extracting into a project in the future, once most of the unused code (and presumably some additional dependency) has been removed.
Alternatively, we could move the whole code into the WooCommerce target itself. However I wouldn't recommend this because that target is already quite big and with tests that take a long time to run.
There is of course a lot of new code in this PR. As you can see from the commit history, I spent some time deleting unused code, but I'm nowhere near done. I decided to draw a line in the sand and ask for review now, though, and continue chipping away at the unused afterwards. Otherwise, this will become stale and we'll risk more work happening in the libraries.
Steps to reproduce
N.A.
Testing information
If CI is green and we can log into the app, then I'd call the operation successful.
Screenshots
N.A.
RELEASE-NOTES.txt
if necessary. – N.A.Reviewer (or Author, in the case of optional code reviews):
Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement: