Skip to content
This repository was archived by the owner on Feb 5, 2025. It is now read-only.

Conversation

@mokagio
Copy link
Contributor

@mokagio mokagio commented Mar 7, 2022

Remove the Lottie third-party library and the screens using it.

These screens were no longer used by the client apps, so the removal is "safe".

Nevertheless, I bumped the major version to represent the breaking change from dropping the Lottie library and consequently removing the UIViewController using it.

Hat tip @jkmassel for having done most of the work already. 🎩

See, both with green unit tests:

@mokagio mokagio mentioned this pull request Mar 8, 2022
3F550D4E23DA429B007E5897 /* AppSelectorTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 3F550D4D23DA429B007E5897 /* AppSelectorTests.swift */; };
3F550D5123DA4A9C007E5897 /* LinkMailPresenter.swift in Sources */ = {isa = PBXBuildFile; fileRef = 3F550D5023DA4A9C007E5897 /* LinkMailPresenter.swift */; };
3F550D5323DA4AC6007E5897 /* URLHandler.swift in Sources */ = {isa = PBXBuildFile; fileRef = 3F550D5223DA4AC6007E5897 /* URLHandler.swift */; };
3F9439BE27D6F9B60067183A /* LoginPrologueViewController.swift in Sources */ = {isa = PBXBuildFile; fileRef = 3F9439BD27D6F9B60067183A /* LoginPrologueViewController.swift */; };
Copy link
Contributor Author

@mokagio mokagio Mar 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, LoginPrologueViewController is required in the codebase despite being used directly by the clients of this library. The reason is that the Login storyboard uses it. Without it, the storyboard fails to decode. I originally removed it but had to put it back and didn't feel it granted a git revert -n dance, which is why it looks moved here.

I think there's space for way more files removal, but I don't want to go overboard with this PR.

I also think it would be fine to ship this as 2.0.0 and then remove more files, even though that might consist as another breaking change. After all, we are the only consumers of this library, and the successful tests done for this PR shows the old login flow code is unused. Also, even though I haven't checked, I'd assume most if not all of the code now removable would be internal. Removing that wouldn't affect clients.

The rationale is to prevent usages outside of the file to facilitate
getting rid of it in the future.
Copy link
Contributor

@AliSoftware AliSoftware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good, but left some very small suggestions for pbxproj format update and use of deprecation annotation; take it or leave it.

"${BUILT_PRODUCTS_DIR}/WordPressKit/WordPressKit.framework",
"${BUILT_PRODUCTS_DIR}/WordPressShared/WordPressShared.framework",
"${BUILT_PRODUCTS_DIR}/WordPressUI/WordPressUI.framework",
"${BUILT_PRODUCTS_DIR}/lottie-ios/Lottie.framework",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth updating the pbxproj's project format version to a more recent one so that it supports using xcfilelist (making CocoaPods then use them and have those changes be outside of the pbxproj)?

Maybe for a separate PR though (similar to the idea you floated around about removing more files later… though the change in pbxproj format would not be a breaking one, so could be worth doing befor a new major version bump…? 🤷 )

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh good point. Thanks. I'll do that in a dedicated PR, to isolated that project structure change.

Comment on lines +5 to +8
// This property is a legacy of the previous UX iteration. It ought to be removed, but that's
// out of scope at the time of writing. It's now `private` to prevent using it within the
// library in the meantime
@objc private var pages: [UIViewController] = []
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// This property is a legacy of the previous UX iteration. It ought to be removed, but that's
// out of scope at the time of writing. It's now `private` to prevent using it within the
// library in the meantime
@objc private var pages: [UIViewController] = []
@available(*, deprecated, message: "This property is a legacy of the previous UX iteration and ought to be removed (but that was out of scope at the time of writing).")
// Property is `private` to prevent using it within the library in the meantime, see deprecation message above
@objc private var pages: [UIViewController] = []

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a nice idea, thanks.

I tried it locally, and got a lot (well, 10) warnings:

image

All the warnings in the mini map are because of the deprecated page.

Part of me welcomes them. "Yes! Make some noise in the build so we get rid of dead code ASAP." Another part of me doesn't want them because, while it's true that we ought to get rid of that code, there's no pressing necessity to do it and I wouldn't want other, legit warnings to get lost among them.

Currently, I'm leaning toward not throwing the warnings, so I'm going to merge as is. Happy to revisit, though.

@mokagio mokagio merged commit a8ba0d9 into trunk Mar 11, 2022
@mokagio mokagio deleted the remove-lottie branch March 11, 2022 04:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants