Circular dependencies / Tangles between modules

[rebornix]

As we introduced the top/primitive components via #8976 and moved the moduels/helpers to their logical folders, we had circular dependencies between the top components. Below is a simplified version of component dependnecies generated by Structure101

We want to gradually remove the dependencies in wrong directions, including

• No dependencies on notebooks/iw/webview in platform
• No dependencies on iw in notebooks
• Move feature code/helpers from telemetry to feature components. notebooks/iw/webview depends on telemetry but not the other way around
• ...

With above items tackled we could end up with an ideal architecture like below (we still need to revisit if it fits)

[rebornix]

While moving towards the ideal infrastructure, we might find our changes/PRs get really large even for fixing a single layer breaking relationship (e.g., remove webviews dependencies from kernels). To ensure us together contribute to the refactoring without stepping on each other's feet too often, we could take steps, from straightforward ones (telemetry, api), to layer protection through eslint rules and lastly a set of new top components.

Separate feature and infra code

• Telemetry should only depend on platform component
  • Move InterpreterPackageTracker and ImportTracker out of telemetry
  • Move sendKernelListTelemetry and kernel types from telemetry to kernels
  • Move splitMultilineString to platform or helper (currently it's imported from webviews)
• API currently consists of core typings and API exposed to external extensions, these should be split into two components
  • Core typings in platform
  • Top component as api. No dependencies from notebooks/kernel/webviews/iw to api
• Logging
  • Remove GitHubIssueCodeLensProvider and GitHubIssueCommandListener out of platform/logging (this was never implemented, might as well remove this dead code) #10360
  • Move LogReplayService out of intellisense

Layer protections

• 🏃 no dependents on interactive-window
  • notebooks does not depend on iw vscode-jupyter#10153
  • no webview dependencies (move webview related helpers into platform)
• notebooks depend on kernels only. kernels depend upon only platform
  • Move Variables related functionalities (which read notebook*, kernels and webview types) out
  • Move ipywidgets message coordination code out of kernel
  • Move debugging/execution common helpers from notebooks to kernel
  • Register Intellisense provider in intellisense

Eslint rules

• telemetry can only use platform
• platform can use nothing else
• kernels can only use platform, telemetry
• notebooks can only use platform and kernels and telemetry
• 🏃 interactive can only use platform, notebook, and kernels and telemetry
• webviews can only use platform and telemetry
• intellisense can only use platform and telemetry
• test can use anything

Extend top components

Currently components which depend on multiple primitive components (kernels/notebooks) are placed in multiple places (i.e., platform/*). We may want to move them out of the primitive components and group them together. These features/components include:

• Variable View
• Plot Viewer
• Ipywidgets
• Commands (Export, SaveSVG, etc)
• API
• Extension recommendation

---

With above tasks finished, we could have eslint rules proposed in #9891 by @rchiodo .

[rebornix]

Discussed with @rchiodo offline about how we measure the success of the refactoring. The strategy we are talking here is clear (mostly adapting the eslint rules #9891 for layer protection and ensure we have minimal bundle size #10307), we would have better code structure and smaller bundle size (meaning faster download and activation) but at the same time we could use a few other metrics to validate how the refactoring goes. This is not scientific and we will explore what other potentials the refined architecture can give us in return.

• Clear component dependencies and architecture diagrams
  • Reference: VS Code's code structure wiki https://github.com/microsoft/vscode/wiki/Source-Code-Organization
  • Structure101 tangle analysis
  • Dependency graph generated through https://github.com/microsoft/vscode-jupyter/tree/main/docs
• Bundling
  • Tree shaking unused third-party npm packages and also ensure desktop-only code are not being bundled into web Reduce web bundle size #10307
  • Dead code removal through https://github.com/jrieken/ts-unused
• Code is easier to contribute to for new beginners
• PRs are relatively smaller and fewer layer breaking. It would take time to measure and compare but what we would love to see is fewer giant PRs touching multiple components (but we will still have quite some for new features)
• Ideas
  • We could cherry-pick layers and bundle them and test specifically against these layers. This can give us a sense of whether our components are decoupled properly and dependent components introduce unwanted side effects on base components

[rebornix]

Update of structure101 analysis

Before (release branch)

Now (main, June 23)

[rebornix]

[rebornix]

With refactoring of the top components and eslint rules established for layers, we now have a wiki page for the source code organization https://github.com/microsoft/vscode-jupyter/wiki/Source-Code-Organization. However a bunch of my refactorings are moving files to where they ideally should be but I didn't always dig deep into the components and figure out if their internal architecture is well decoupled and tangle-less.

By using structure101, here is what each component looks like now:

platform

kernels

notebooks

interactive-window

standalone

[DonJayamanne]

Does it matter how the internal code of the individual components reference each other?
I didn't think they are not components in their own right, hence I do'nt think they are governed by the same rules.
E.g. does it matter if the languages & telemetry have references to controllers in nbotebooks & vice versa in the notebooks components?

Probably something for discussion in standup.

[rebornix]

Does it matter how the internal code of the individual components reference each other?

My short answer is not always. The tangles from structure101 can be used as indicators for how decoupled a component is and can be used as one approach to improve the code. There are good discussions in VS Code microsoft/vscode#140874 about if/why/how of using structure101 to analyze and improve VS Code's code base.

I didn't think they are not components in their own right, hence I do'nt think they are governed by the same rules.

I agree and I don't think we should enforce any layering concepts inside top components. 0 tangle is also not what we would try to archive. The goal IMHO should be better readability.

Based on my experience of the last two weeks, the tangles shown by structure101 (or a super-restrict eslint rule) helped find code that are placed in wrong folders/modules.

[DonJayamanne]

@rebornix please could you check if there's more work remaining here, if there is please could you document this so that we can resolve them and close this issue

[rebornix]

@DonJayamanne we can sit together and revisit this issue later this month.

[DonJayamanne]

Given that we're not doing anything actively in this space, lets close this for now.