Upgrade to core.async 1.6.673 #231
Conversation
Some of the functions that we were relying on to hijack the go state machine compilation changed namespaces. This updates the code to reference where they now live. Addresses clj-commons#229
There was a problem hiding this comment.
Wow, that was fast!
Unfortunately, since not everyone uses core.async with manifold, it's declared scope :provided, which means they choose which version to bring. (And even without scope provided, users could always override the core.async dep.)
The solution needs to check both namespaces where the vars could be defined, and then use the one it finds.
|
Sorry for the slow response here, @KingMob . I've been thinking this over and I'm not sure that the complications of implementation make it worth supporting both. I think having a minimum version pinned as our provided scope, and warning in the release that if you upgrade to this version there's a particular minimum version you're expected to have of core.async. At least, my interpretation of provided dependencies has always been, "We tested with this particular version of the library. Caveat emptor if you choose to use a different version." Let me know your thoughts? |
|
Hmmm. I guess my problem with "Caveat emptor if you choose to use a different version" is that usually means "We haven't tested with your version, and we can't predict what will happen". But in this case, we know it will break, and we know how to fix it. It feels like we're punting a problem to the user. OTOH, a quick Github search doesn't show anyone using it, so... ¯\_(ツ)_/¯ I think, to be safe, I'd still rather support both namespaces. |
|
@KingMob , I've validated locally that the change I made works with both the current & older version of core.async, but I'm unsure the best way to set up circleci to actually test both |
|
Great work! To test both, maybe add two extra profiles in project.clj? One I think that would work |
tanzoniteblack
force-pushed
the
ryan/core-async-1.6.673
branch
from
c1f53b4 to
ef9038a
Compare
|
@KingMob , I added a new profile explicitly for the old core.async version and have the default deps list using the newer version, rather than adding profiles for both. Circleci config has also been updated to run the go-off tests. I've verified locally that the different profiles are correctly using the right core.async versions. |
|
Fixes #229 |
Some of the functions that we were relying on to hijack the go state machine compilation changed namespaces. This updates the code to reference where they now live.
Addresses #229