Break the cyclic dependency between BaseTarget and UntypedTarget. #2037
Conversation
…rom many target types.
bjaspan
changed the title
|
Hmm, there's a fair amount of copy paste here. Is there a reason we keep |
|
Yeah I'm thinking maybe the right answer is to just put those in the same file? That seems less onerous than this approach. Still don't love it, but at least better than this. @auscompgeek what's your opinion on circular deps? |
|
I tried merging BaseTarget and UntypedTarget as well. It turns out that UntypedTarget depends on PlainTarget (via TokenInsertionRemovalBehavior) which also depends on BaseTarget. So, to solve the problem that way, we'd have to merge BaseTarget.ts, UntypedTarget.ts, and PlainTarget.ts into a single file. I'm fine with that solution; I went with this one only because I guessed that you would all prefer it to merging three files. |
|
Hold on. There is still a cyclic dependency between UntypedTarget.ts and createContinuousRange.ts. |
|
I removed the cyclic dependency between UntypedTarget.ts and createContinuousRange.ts. It would be nice if we had a tool to create a dependency graph, but I believe what we have now is:
|
|
#2047 is the merging-files approach. |
…eateContinuousRangeTarget uses createContinuousRangeOrUntypedTarget.
|
I've eliminated the copy-paste by introducing a new abstract base template class, CommonTarget, which provides the createContinuousRangeOrUntypedTarget functionality. By exporting it from UntypedTarget, the import graph described in my previous comment is preserved. |
There was a problem hiding this comment.
Yeah this looks fine modulo a couple comments. Would be great to get high-level feedback from @auscompgeek if possible, but if too busy we can move forward anyway
| * a createContinuousRange if this and endTarget have the same type or | ||
| * otherwise returns an UntypedTarget from this to endTarget. | ||
| */ | ||
| export abstract class CommonTarget< |
There was a problem hiding this comment.
I think I may call this CommonBaseTarget? Idk wdyt @auscompgeek
There was a problem hiding this comment.
CommonBase sounds like a bit of a tautology to me 😄
There was a problem hiding this comment.
What about CommonContinuousRangeTarget? Any name is fine with me. :-)
|
closing in favour of #2074 |
This PR factors
createContinuousRangeTargetout of BaseTarget into a new function,createContinuousRangeOrUntypedTarget, which is used by a variety of target types. As a result, BaseTarget.ts no longer needs to import UntypedTarget (indirectly through createContinuousRange.ts).