Skip to content
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

Remove tca in favor of combine-only approach to core location dependencies #40

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

klundberg
Copy link
Contributor

The prior PR I pushed fixed up usage of the library with TCA 0.57-0.59, but using the library still results in warnings that cannot be resolved in apps that use it (namely, that effects conforming to publisher is deprecated). In an effort to remove warnings from apps that use the library, I figured a refactor to just use combine types instead of effectpublisher would be a useful thing to have. Is this something that might be desired in an official release of this library?

I opted to not go the AsyncSequence route since there's not an official way to create a shared stream with multiple subscribers yet, and I suspect that accidentally creating multiple subscribers to one for this library would cause a lot of hard to diagnose issues.

If this is a welcome change, the demo project will still need updating to get CI to work but I'm happy to do that too.

@andtie
Copy link

andtie commented Sep 8, 2023

Hi @klundberg! Thanks a lot for putting in the effort to keep this library updated! I like the idea to decouple it from TCA to reduce the maintenance burden.

Currently, my main objective is to be able to move to the latest TCA-version. But even with this PR, the dependency on xctest-dynamic-overlay with version "0.9.0" prohibits this. Is there a specific reason to use the old version?

@klundberg
Copy link
Contributor Author

@andtie it's set to "from 0.9.0" so that it can use that or any newer version. does SPM complain about using this update with newer versions of tca and xctest-dynamic-overlay?

@klundberg
Copy link
Contributor Author

Ah my mistake, i didn't understand that from: ... was equivalent to upToNextMajor. I've updated the version to include a range that should also allow for 1.x versions @andtie

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.

2 participants