-
Notifications
You must be signed in to change notification settings - Fork 121
Optimize extensions SPM setup and symbols removal #15884
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
Conversation
Generated by 🚫 Danger |
|
|
|
Thanks for working on this Ian! I ran the app and did general smoke testing and found no issues, I've also specifically tested notifications and the usages of
@joshheald @itsmeichigo since you have Apple watches, would you mind to try and run the app and play around with it (trigger an order notification as well, see if there's any problem when marking it as read), see if we have any trouble? One would be enough 👍
Very interesting, this seems to be related or a side-effect from a separate issue: WOOMOB-772. in here we found that the watch version of the app requires this conditional module imports because skips the usage of Yosemite, so I wonder if once we remove this requirement (there's a task to check if we can simply duplicate the code for these 3 files that are currently shared) we'll see further reduction in the app size since there would also be less repetition of dynamic frameworks being bundled with the binary when not needed 🤔
Nice, from this issue on Stripe's repo, I wonder if we'll see an additional ~20MB save just by doing this. No way of really knowing until the build goes through app release, right? I'll wait until we've tested with watch before approving! |
Indeed, definitely related. But I don't think there are more app targets including the
It will definitely make it decrease as well as other Frameworks. I tested it by creating an Archive and comparing the size of the content inside the archive (which is still pre-thinning and uncompressed). That way, the extensions were ~18MB smaller, the Also, please note that these changes were non-exhaustive, there are probably still ways to further optimize the SPM setup and dependency graph.
Thank you! 🙇 |
I tested with the prototype build and the watch app works for me (no crashes). I didn't receive any notification on either the phone or the watch - I hope it's an issue with the alpha build and not from the changes in this PR. |
Thanks for testing! Notifications should be unaffected. I did had troubles to receive them until I realized that my test site was not connected to Jetpack, then I started to receive them again. Perhaps that's the issue in your end as well? I'll approve this one for the moment since the changes seem to cause no new crashes in the watch 👍 |

Fixes AINFRA-888
Related: p1751876745034679-slack-CC7L49W13
Description
Redundant SPM Dependencies
After the work to consolidate Swift packages such as #15651, we've noticed a substantial increase in app size.
After an investigation, I've noticed the extensions increased quite a bit between app versions:

Purely based on the extensions code and their imports, it seemed that a couple of dependencies declared in
Package.swiftwere redundant. After removing the dependencies, I wasn't able, though, to generate regular development builds, as seen on Buildkite.I then figured that the file
MarkOrderAsReadUseCase.swiftwas being included in the NotificationService extension, and the extension depended on theYosemiteframework.There was already logic to conditionally import
YosemitewithcanImport(Yosemite), but it seemscanImportdoesn't work with SPM libraries (see related discussion), and perhaps that's the reason why the dependency had to be added. I then removed the dependency but had to introduce the flagNOTIFICATION_EXTENSION.There are alternatives to this approach, but they would be a bit more involved (like extracting the
MarkOrderAsReadfunctionality entirely to the notification extension), but for now I went for an approach requiring minimal changes.After removing those dependencies, I've noticed a reasonable decrease in the extensions'
.appexfile sizes, which should be reflected in the final app size.Strip Debug Symbols During Copy
In addition to that, I've noticed we had the project flag
COPY_PHASE_STRIPset toNOeven for release builds (the default isYES). This should impact the frameworks we include in the app (such asStripeTerminalwhich unstripped is around 41MB alone). AboutCOPY_PHASE_STRIP:Note
Please note that these updates are non-exhaustive and probably there are still ways to further optimize the SPM setup and dependency graph.
Testing information
We should check the prototype builds are still fine, particularly the extensions.
We need also make sure the extensions are working fine in other releases in the regular cycle (Beta, Release).
RELEASE-NOTES.txtif necessary.