Skip to content

chore: remove code from WireDomain package - WPB-14598 #2945

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

Closed

Conversation

caldrian
Copy link
Contributor

@caldrian caldrian commented Apr 25, 2025

TaskWPB-14598 [iOS] Add shared backup library as a dependency

Issue

This PR removes the more or less redundant WireDomain package, but keeps the WireDomain Xcode project.

Testing

Describe how to test.

Optional: attachments like images, videos, etc.


Checklist

  • Title contains a reference JIRA issue number like [WPB-XXX].
  • Description is filled and free of optional paragraphs.
  • Adds/updates automated tests.

UI accessibility checklist

If your PR includes UI changes, please utilize this checklist:

  • Make sure you use the API for UI elements that support large fonts.
  • All colors are taken from WireDesign.ColorTheme or constructed using WireDesign.BaseColorPalette.
  • New UI elements have Accessibility strings for VoiceOver.

Copy link
Contributor

github-actions bot commented Apr 25, 2025

Test Results

    6 files    502 suites   4m 29s ⏱️
3 307 tests 3 148 ✅ 28 💤 131 ❌
3 317 runs  3 159 ✅ 28 💤 130 ❌

For more details on these failures, see this check.

Results for commit af187e2.

♻️ This comment has been updated with latest results.

@caldrian caldrian enabled auto-merge (squash) April 25, 2025 13:26
@caldrian caldrian requested a review from samwyndham April 25, 2025 13:46
Copy link
Contributor

@samwyndham samwyndham left a comment

Choose a reason for hiding this comment

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

Hey @caldrian

In the current PR I'm working on I add a new target to WireDomain SPM.

Basically for the new syncing we duplicate many models from WireDomain just for the purpose of decoding/encoding into data. This is to prevent an innocent change to something like QualifiedID such as the rename of property from breaking the decoding of stored UpdateEvents. I would like to keep these models in a separate target from the main WireDomain target because the public API is tiny and I don't want to confuse things in that target. I can certainly move my code elsewhere but can we wait until next week before merging this PR to discuss a bit further. I don't really have time today to look into it

@caldrian caldrian disabled auto-merge April 25, 2025 14:53
@wireapp wireapp deleted a comment from datadog-wireapp bot Apr 25, 2025
@caldrian
Copy link
Contributor Author

Hey @caldrian

In the current PR I'm working on I add a new target to WireDomain SPM.

Basically for the new syncing we duplicate many models from WireDomain just for the purpose of decoding/encoding into data. This is to prevent an innocent change to something like QualifiedID such as the rename of property from breaking the decoding of stored UpdateEvents. I would like to keep these models in a separate target from the main WireDomain target because the public API is tiny and I don't want to confuse things in that target. I can certainly move my code elsewhere but can we wait until next week before merging this PR to discuss a bit further. I don't really have time today to look into it

Reverted deleting the target.

@caldrian caldrian changed the title chore: delete WireDomain package - WPB-14598 chore: remove code from WireDomain package - WPB-14598 Apr 25, 2025
@caldrian caldrian marked this pull request as draft April 28, 2025 05:08
@caldrian caldrian force-pushed the chore/delete-wiredomainpackage-WPB-14598 branch from af187e2 to fb482cb Compare April 28, 2025 05:08
caldrian added a commit that referenced this pull request Apr 28, 2025
This reverts commit 824f9c2.

# Conflicts:
#	WireUI/Tests/WireSettingsUITests/Account/BackupImportExport/Import/ImportBackupViewModelTest.swift
#	wire-ios-sync-engine/Source/Use cases/ImportBackupUseCase/ImportBackupFileArchiver.swift
#	wire-ios-sync-engine/Source/Use cases/ImportBackupUseCase/ImportLegacyBackupUseCase.swift
#	wire-ios-sync-engine/Source/Use cases/ImportBackupUseCase/SessionManager+importBackupUseCase.swift
#	wire-ios-sync-engine/Support/Sourcery/generated/AutoMockable.generated.swift
#	wire-ios/Wire-iOS/Sources/UserInterface/Settings/CellDescriptors/SettingsCellDescriptorFactory+Account.swift
caldrian added a commit that referenced this pull request Apr 28, 2025
This reverts commit 824f9c2.

# Conflicts:
#	WireUI/Tests/WireSettingsUITests/Account/BackupImportExport/Import/ImportBackupViewModelTest.swift
#	wire-ios-sync-engine/Source/Use cases/ImportBackupUseCase/ImportBackupFileArchiver.swift
#	wire-ios-sync-engine/Source/Use cases/ImportBackupUseCase/ImportLegacyBackupUseCase.swift
#	wire-ios-sync-engine/Source/Use cases/ImportBackupUseCase/SessionManager+importBackupUseCase.swift
#	wire-ios-sync-engine/Support/Sourcery/generated/AutoMockable.generated.swift
#	wire-ios/Wire-iOS/Sources/UserInterface/Settings/CellDescriptors/SettingsCellDescriptorFactory+Account.swift
Copy link
Contributor

@samwyndham samwyndham left a comment

Choose a reason for hiding this comment

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

Nice work 🧹

@caldrian caldrian deleted the branch release/cycle-3.123 April 29, 2025 09:47
@caldrian caldrian closed this Apr 29, 2025
@caldrian caldrian reopened this Apr 29, 2025
@jullianm
Copy link
Contributor

jullianm commented Apr 29, 2025

@caldrian question: I assume we can't solely keep the Swift package and remove the Xcode project because it relies on other Xcode framework like WireDataModel and WireTransport, right?

thought: My concern is that we gradually adopt Swift packages for new code, I wonder is there's a possibility to do the other way around : keep the WireDomain package, remove the Xcode framework and embed legacy frameworks into Swift packages (as binaryTarget?) that WireDomain package could use. (probably out of scope of this PR as it would require much more effort, just a thought)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants