Resolve cyclic imports

[henrymercer]

We ran into an issue during the rollout of the new analysis summary feature where the version check wasn't working properly. This resulted in us trying to pass a CLI argument to a CodeQL version that didn't understand it. This bug occurred due to a cyclic import affecting codeql.ts and feature-flags.ts.

Since this isn't the first time we've ran into this sort of issue, I figured it was worth introducing some static checking to try to prevent this kind of problem from happening again.

This PR enables the "no cyclic imports" eslint rule, and fixes all the cyclic imports it reports. Commit-by-commit review strongly recommended as this took a lot of refactoring!

Merge / deployment checklist

• Confirm this change is backwards compatible with existing workflows.
• Confirm the readme has been updated if necessary.
• Confirm the changelog has been updated if necessary.

[aeisenberg]

Thanks for doing this. I think it is a good change. The main thing I am learning about this is that we have too much stuff in our util.ts module. My thinking is that we should avoid generic util modules and put code into smaller, more focused modules. It makes things easier to find, and also less likely to have circular dependencies.

[henrymercer]

I mostly agree, with the main caveat being when we need to reference a function from two files A and B where A depends on B or B depends on A, it's often easier to put it in util.ts rather than creating a dedicated file. One example from this PR is getMlPoweredJsQueriesPack().

Another thing I took from the PR was around layering. We have a bunch of functions that call the API somewhat opaquely via getApiClient() — the API client isn't a parameter. This means we had a lot of hidden dependencies on api-client across the codebase. I think this linting rule will enforce a better layering where API calls live in a more specific set of modules.

[aeisenberg]

I mostly agree, with the main caveat being when we need to reference a function from two files A and B where A depends on B or B depends on A, it's often easier to put it in util.ts rather than creating a dedicated file. One example from this PR is getMlPoweredJsQueriesPack().

This further reinforces my point that util is just a place to put a bunch of unrelated stuff. 😄 It might be convenient to put things there, but it doesn't help make the architecture any clearer.

Another thing I took from the PR was around layering. We have a bunch of functions that call the API somewhat opaquely via getApiClient() — the API client isn't a parameter. This means we had a lot of hidden dependencies on api-client across the codebase. I think this linting rule will enforce a better layering where API calls live in a more specific set of modules.

Yes. Though, it could make things somewhat awkward because we'll start to need to pass in the API Client into most functions in the action. It might be worth trying the refactoring to see how bad it gets. This would be a good use case for dependency injection (but that is a whole other can of worms).