polylith
polylith copied to clipboard
polyfy
poly check treats :as-alias namespace as loaded
Describe the bug
A namespace referenced purely for an alias, via :as-alias, does not cause a circular reference, but poly check incorrectly treats it as fully-loading the namespace (it should not).
Will provide more details shortly, along with a PR since this is a critical blocker for us at work.
Okay, thanks for the info. One idea is that you create a small workspace in examples, that reproduces the problem, which I can use to check my solution.
I haven't had time to do this yet but might have time this weekend. I have a couple of other, very minor fixes to propose as well so I may send PRs for those soon too.
Here's the change we made in our fork to simply "ignore" requires that use :as-alias (at least in terms of loaded libs).
This isn't ideal (per discussions on Slack) because it prevents poly from checking that only interfaces are used in this case. A complete solution is likely to involve quite a bit of rework in Polylith because it currently assumes that the list of required nses can be used for two purposes: checking the "rules" of component usage (good, still wanted in this case), and figuring out the dependency graph (not true in this case: :require [top-ns.component.interface :as-alias thing] does not contribute to the dependency graph -- it only introduces an alias, as if ns-alias were called).
I may have a simple solution to this. Will look into this today @seancorfield.
I have hopefully fixed the problem, and you can have a look at the issue-247 branch, @seancorfield.
If the code refers to another component interface using :as-alias then the :interface-deps will not be populated with that dependency, which solves the problem with circular dependencies. However, if the code refers to an implementing namespace using :as-alias then it will be included, and we will get both error 101 (Dependency on implementing namespace) and error 104 (circular dependency). The second error is in this case not correct, but you have to fix the error 101 first anyway, and it simplified the implementation a lot.
That sounds like a pretty slick solution to the problem. I'll try to find time over the weekend to put together a branch based on issue-247 but with our classloader and GC patches applied and then try it on Monday at work.
OK, I merged your issue-247 branch into a new branch in my fork (as-alias-247) and then cherry-picked my classloader and GC commits onto that, and I've set a reminder for Monday for me to test that as work. I should have feedback for you pretty quickly at that point.
The PR for this has been merged so this issue and #211 are both completed.