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

Adds support for type-fn multimethods #60

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jelmerderonde
Copy link

This PR makes it possible to have multimethods as type-fns. It's pretty basic, I didn't think any more changes were needed. Let me know If there's something else to do.

@vlaaad
Copy link
Contributor

vlaaad commented Mar 18, 2020

I've been thinking about it, and what worries me is that it might be a breaking change. Users might have their own fx->lifecycle function like that:

{:fx.opt/type->lifecycle #(or (fx/keyword->lifecycle %)
                              (fx/fn->lifecycle %)
                              (when (instance? clojure.lang.MultiFn %)
                                my-custom-multi-fn-lifecycle))}

That way after update their apps will break because MultiFns will suddenly start using different lifecycles.
I think a non-breaking way to add MultiFn support is somehow checking for MultiFns after using type->lifecycle. It could be done by extending the Lifecycle protocol to multi-methods.

I just realized there is another problem: applications that use contexts have different lifecycles for functions, so there is another place to update: fx/fn->lifecycle-with-context. Sorry, I didn't realize that adding support for multi-methods is much more complex...

@vlaaad
Copy link
Contributor

vlaaad commented Mar 18, 2020

I'll need some more time to think about MultiFn support, extending Lifecycle protocol won't work for them since there needs to be 2 different behaviors depending on whether contexts are used or not.

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.

None yet

2 participants