-
-
Notifications
You must be signed in to change notification settings - Fork 14
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
Allow external ownership for Modules #106
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #106 +/- ##
==========================================
- Coverage 87.93% 87.64% -0.28%
==========================================
Files 42 46 +4
Lines 1217 1375 +158
==========================================
+ Hits 1070 1205 +135
- Misses 147 170 +23
Continue to review full report in Codecov by Sentry.
|
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.
Nice work @Supereg; great addition to the overall system design for Spezi!
Only major point I would see is the SwiftLint dependency for the main package. I would propose to rather move that to the UI Testing Application as the main point to develop the package, run all tests, and display linting warnings inside. Let me know what you think and how you want to proceed here.
We might need to wait for SPM to become smarter and not checking out and/or building buildtime or testing dependencies when using a SP as a dependency ...
Sources/Spezi/Capabilities/Communication/CollectedModuleValues.swift
Outdated
Show resolved
Hide resolved
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.
Thank you for the improvements; looks great!
Allow external ownership for Modules
♻️ Current situation & Problem
When loading modules dynamically using
loadModule(_:)
, Spezi always takes ownership of the Module. Spezi keeps a strong reference untilunloadModule(_:)
is called. This isn't strictly necessary (with some conditions applying). In some cases it is helpful for framework engineers to not pass ownership to Spezi. Namely, one can make use of being notified when objects are de-initialized (e.g., a framework creating a Module and passing ownership to the framework user and relying on the deinit to be notified when the user deallocated the object).This PR introduces the concept of
ModuleOwnership
that can be passed to the updatedloadModule(_:ownership:)
method.external
ownership is available to framework developers via theAPISupport
SPI.When specifying
external
ownership does not keep any strong references to the loaded Module. This required making some changes how resources are automatically cleared (e.g.,@Dependency
,@Provide
,@Modifier
or@Model
modifiers automatically clean up their resources once their are deallocated). Some features likeEnvironmentAccessible
are not available when usingexternal
ownership as they would inherently imply strong ownership.Other modules that depend on such modules either maintain a weak reference if they declare it as an optional dependency (in this case the dependency is automatically removed once the Module de-initializes) or a strong reference if declared as a required dependency. In such a case, ownership is shared with that module for the lifetime of the module that declares a required dependency to an externally managed module.
⚙️ Release Notes
📚 Documentation
Documentation was updated for changed interfaces.
✅ Testing
Additional unit testing was added to verify functionality of dynamically loaded modules with external ownership.
📝 Code of Conduct & Contributing Guidelines
By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines: