Skip to content

MBL-2658: Move design system code into new package named KDS#2598

Merged
amy-at-kickstarter merged 22 commits into
mainfrom
feat/adyer/mbl-2658/kds-spm
Aug 28, 2025
Merged

MBL-2658: Move design system code into new package named KDS#2598
amy-at-kickstarter merged 22 commits into
mainfrom
feat/adyer/mbl-2658/kds-spm

Conversation

@amy-at-kickstarter

@amy-at-kickstarter amy-at-kickstarter commented Aug 21, 2025

Copy link
Copy Markdown
Contributor

📲 What

Move our all our new design system code into a local Swift package, named KDS.

🤔 Why

  • Pulling out domain-specific code into separate packages should improve our build times, by flattening our (Very vertical) dependency structure.
  • Modularizing code forces us to think more carefully about the dependencies it requires, ultimately forcing us to make smarter decisions about class/method visibility and dependencies.
  • The design system is a neatly packaged set of features, so it is a great first candidate for this kind of modularization.
  • SPM packages are nifty and mean we don't need to individually define files in our Xcode project.

🛠 How

I learned some interesting things throughout this process - these are called out specifically in code comments.

👀 See

Screenshot 2025-08-21 at 5 59 25 PM

@amy-at-kickstarter amy-at-kickstarter changed the title [WIP] MBL-2658: Move design system code into new package named KDS MBL-2658: Move design system code into new package named KDS Aug 21, 2025
Comment thread KDS/Package.swift
)
],
swiftLanguageModes: [
.v5

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Specifically calling out this package as Swift 5.0 and iOS 16.0.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems inconsistent with the swift-tools-version: 6.0 at the top of the file (commented out). Can you explain why they should both be here or delete one of them?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Just kidding, that breaks on CircleCI! Somewhat confusing why there are two definitions in this file, but looks like it's necessary.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Weird! Thanks for trying though

}

extension UIColor {
public extension UIColor {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I went down a really deep rabbit hole of figuring out which methods (specifically around color generation) should be public, and which should not be.

It started when I made UIColor.hex internal to the KDS package. This was actually really nifty, because it automatically flagged the handful of places in the code where we're manually creating colors off of the design system! But then I went pretty deep trying to figure out how to handle these one-off color cases...and eventually I came around to just making the method public again.

I wanted to highlight this because it is a very powerful tool for enforcing code conventions - you can be really intentional about only exposing a small surface area from your package/library, in a way that we typically don't bother to do.

Comment thread KDS/Sources/KDS/Fonts/CustomFont.swift Outdated
}

var error: Unmanaged<CFError>?
CTFontManagerRegisterFontsForURL(fontURL as CFURL, .process, &error)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Spiffy trick I found for registering fonts. This meant we can remove the Fonts section from Info.plist.

Comment thread KDS/Sources/KDS/Fonts/CustomFont.swift Outdated

var error: Unmanaged<CFError>?
CTFontManagerRegisterFontsForURL(fontURL as CFURL, .process, &error)
assert(error == nil, "Error registering font: \(String(describing: error))")

@amy-at-kickstarter amy-at-kickstarter Aug 21, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I went down another rabbithole making sure asserts work as we expect - they do, in fact, run in debug builds, but are stripped from release builds.

It would be good to verify if this is because SPM inherits build flags from the target building the SPM module (in this case, Library), or if SPM automatically adds the flag to strip assertions.

You can add custom swift build flags to an SPM module, so it's not totally clear to me how the inheritance of flags works.

Comment thread Kickstarter-iOS/AppDelegate.swift Outdated
UIImageView.appearance(whenContainedInInstancesOf: [UITabBar.self])
.accessibilityIgnoresInvertColors = true

InterFont.registerFont()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I believe we can now add this to Kickstarter-Framework-Tests and get our screenshot tests all building with Interfont.

import FBSDKCoreKit
import Firebase
import Foundation
import KDS

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Downside of modularization: 300 files that just add import KDS.

|> UITextField.lens.returnKeyType .~ .done
}

private let savePasswordButtonStyle: ButtonStyle = { button in

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Unused, deleted because it required mixLighter which I made internal to KDS.

Comment thread Kickstarter-iOS/Info.plist Outdated
<false/>
<key>GOOGLE_ANALYTICS_DEFAULT_ALLOW_AD_PERSONALIZATION_SIGNALS</key>
<false/>
<key>UIAppFonts</key>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

See note above about fonts. Perhaps we should leave this line, though?

ReferencedContainer = "container:Kickstarter.xcodeproj">
</BuildableReference>
</TestableReference>
<TestableReference

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is one weird bit of SPM I can't quite figure out. I can add the KDS scheme, and we have a target KDSTests, but I can't just run KDSTests. Instead, I had to add them to another schema, in this case Library-iOS. I think this is because Xcode assumes an SPM module is an external dependency, and why would you want to test an external dependency all the time?

I'm not sure if there's a better workaround. Maybe using "Test Plans"? Thoughts welcome.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK, I did figure this out. I had to add a schema to KDS, not to Kickstarter. You can now run the KDS tests from the Kickstarter project using CMD+U.

I still left this added to the Library schema, because I don't want to build a whole new CircleCI job just for these tests.

import UIKit

// This extension contains custom semantic colors that are not part of the official Figma design set.
// This extension contains custom semantic colors for PLOT that are not part of the official Figma design set.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

These all are related to PLOT, so I rescoped them and left them in Library instead of KDS.

@amy-at-kickstarter amy-at-kickstarter requested review from a team, ifosli, scottkicks and stevestreza-ksr and removed request for a team August 21, 2025 22:10
@amy-at-kickstarter amy-at-kickstarter marked this pull request as ready for review August 21, 2025 22:10
@nativeksr

Copy link
Copy Markdown
Collaborator
1 Warning
⚠️ Big PR

Generated by 🚫 Danger

@ifosli ifosli left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good! I have no suggestions for how to make things better (outside of the swift version comment) but I appreciate all your comments, as always!

Comment thread KDS/Package.swift
)
],
swiftLanguageModes: [
.v5

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems inconsistent with the swift-tools-version: 6.0 at the top of the file (commented out). Can you explain why they should both be here or delete one of them?

@amy-at-kickstarter amy-at-kickstarter changed the title MBL-2658: Move design system code into new package named KDS [WIP] MBL-2658: Move design system code into new package named KDS Aug 27, 2025
@amy-at-kickstarter amy-at-kickstarter force-pushed the feat/adyer/mbl-2658/kds-spm branch from 09c105b to e86edc9 Compare August 27, 2025 17:43
@amy-at-kickstarter amy-at-kickstarter changed the base branch from main to feat/adyer/mbl-2658/load-fonts-in-bundle August 27, 2025 17:44
ReferencedContainer = "container:Kickstarter.xcodeproj">
</BuildableReference>
</MacroExpansion>
<EnvironmentVariables>

@amy-at-kickstarter amy-at-kickstarter Aug 28, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

When you call Bundle.module, it looks for bundles inside the framework (like $(BUILT_PRODUCTS_DIR)/Library.framework/KDS_KDS.bundle). However, when you're building the framework alone (i.e. for tests), the bundle isn't copied there. By setting this environment variable in Debug, we can tell the compiler to look for the bundle in $(BUILT_PRODUCT_DIRS)/KDS_KDS.bundle instead.

This is necessary to keep our tests from crashing when you call Bundle.module.
Screenshot 2025-08-28 at 10 54 12 AM

Base automatically changed from feat/adyer/mbl-2658/load-fonts-in-bundle to main August 28, 2025 15:49
@amy-at-kickstarter amy-at-kickstarter changed the base branch from main to feat/adyer/mbl-2658/fix-hardcoded-colors August 28, 2025 17:26
@amy-at-kickstarter amy-at-kickstarter force-pushed the feat/adyer/mbl-2658/kds-spm branch from 71724bc to 7c92cfa Compare August 28, 2025 17:26
@amy-at-kickstarter amy-at-kickstarter changed the title [WIP] MBL-2658: Move design system code into new package named KDS MBL-2658: Move design system code into new package named KDS Aug 28, 2025
@amy-at-kickstarter amy-at-kickstarter force-pushed the feat/adyer/mbl-2658/fix-hardcoded-colors branch 2 times, most recently from ebc26ee to 0041ef7 Compare August 28, 2025 17:55
@amy-at-kickstarter amy-at-kickstarter force-pushed the feat/adyer/mbl-2658/kds-spm branch from 7d00e20 to 75b297b Compare August 28, 2025 18:01
Comment thread KDS/Package.swift
)
],
swiftLanguageModes: [
.v5

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Weird! Thanks for trying though

Base automatically changed from feat/adyer/mbl-2658/fix-hardcoded-colors to main August 28, 2025 21:33
@amy-at-kickstarter amy-at-kickstarter force-pushed the feat/adyer/mbl-2658/kds-spm branch from 75b297b to da4ddf4 Compare August 28, 2025 21:37
@amy-at-kickstarter amy-at-kickstarter merged commit c0f2489 into main Aug 28, 2025
5 checks passed
@amy-at-kickstarter amy-at-kickstarter deleted the feat/adyer/mbl-2658/kds-spm branch August 28, 2025 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants