Skip to content

Detect and fix cyclic runtime dependencies #251003

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

Merged
merged 23 commits into from
Jun 11, 2025
Merged

Conversation

jrieken
Copy link
Member

@jrieken jrieken commented Jun 9, 2025

Background historically we have never allowed to have cyclic runtime dependencies between our modules. The AMD loader would simply fail during load. Since the advent of ESM this has changed, browsers handle cycling dependencies graceful and things might work or not - it depends on the circumstances in which things get loaded (see #250990 for some fun). To prevent this madness, we have added a check for cyclic runtime dependencies (as part of the ESM work). Unfortunately, this check was broken and a couple of cyclic dependencies have slipped in. They should be cleaned-up ASAP

You can run yarn watch-client from this branch/PR to get this list of errors below. I have already done some simple cleanup but need help with the following:

  • CYCLIC dependency: vs/workbench/contrib/chat/common/chatModel.js -> vs/workbench/contrib/chat/common/chatParserTypes.js -> vs/workbench/contrib/chat/common/chatModel.js @connor4312
  • CYCLIC dependency: vs/editor/common/tokens/lineTokens.js -> vs/editor/common/tokens/tokenArray.js -> vs/editor/common/tokens/lineTokens.js @hediet
  • CYCLIC dependency: vs/editor/common/core/text/positionToOffset.js -> vs/editor/common/core/edits/textEdit.js -> vs/editor/common/core/text/abstractText.js -> vs/editor/common/core/text/positionToOffset.js @hediet
  • CYCLIC dependency: vs/editor/browser/gpu/raster/glyphRasterizer.js -> vs/editor/browser/gpu/viewGpuContext.js -> vs/editor/browser/gpu/atlas/textureAtlas.js -> vs/editor/browser/gpu/raster/glyphRasterizer.js @Tyriar
  • CYCLIC dependency: vs/editor/common/model/tokens/treeSitter/treeSitterSyntaxTokenBackend.js -> vs/editor/common/model/tokens/treeSitter/treeSitterTree.js -> vs/editor/common/model/tokens/treeSitter/treeSitterSyntaxTokenBackend.js @hediet @alexr00
  • CYCLIC dependency: vs/workbench/contrib/notebook/browser/notebookBrowser.js -> vs/workbench/contrib/notebook/browser/diff/notebookDiffEditor.js -> vs/workbench/contrib/notebook/browser/notebookEditorWidget.js -> vs/workbench/contrib/notebook/browser/notebookBrowser.js @rebornix
  • CYCLIC dependency: vs/workbench/contrib/chat/browser/actions/chatActions.js -> vs/workbench/contrib/chat/browser/chatEditorInput.js -> vs/workbench/contrib/chat/browser/actions/chatActions.js @roblourens
  • CYCLIC dependency: vs/workbench/contrib/scm/browser/scmHistory.js -> vs/workbench/contrib/scm/browser/util.js -> vs/workbench/contrib/scm/browser/scmHistory.js @lszomoru
  • CYCLIC dependency: vs/workbench/contrib/chat/browser/chatContentParts/chatAttachmentsContentPart.js -> vs/workbench/contrib/chat/browser/chatAttachmentWidgets.js -> vs/workbench/contrib/chat/browser/chatContentParts/chatAttachmentsContentPart.js @jrieken
  • CYCLIC dependency: vs/workbench/contrib/chat/browser/chatAttachmentResolve.js -> vs/workbench/contrib/chat/browser/chatPasteProviders.js -> vs/workbench/contrib/chat/browser/contrib/chatDynamicVariables.js -> vs/workbench/contrib/chat/browser/chatWidget.js -> vs/workbench/contrib/chat/browser/chatAccessibilityProvider.js -> vs/workbench/contrib/chat/browser/actions/chatToolActions.js -> vs/workbench/contrib/chat/browser/actions/chatToolPicker.js -> vs/workbench/contrib/mcp/browser/mcpCommands.js -> vs/workbench/contrib/mcp/browser/mcpResourceQuickAccess.js -> vs/workbench/contrib/chat/browser/chatAttachmentResolve.js @connor4312
  • CYCLIC dependency: vs/workbench/contrib/extensions/browser/extensionsViewer.js -> vs/workbench/contrib/extensions/browser/extensionsViews.js -> vs/workbench/contrib/extensions/browser/extensionsViewer.js @sandy081
  • CYCLIC dependency: workbench/contrib/chat/common/promptSyntax/codecs/base/frontMatterCodec/parsers/frontMatterValue.js -> workbench/contrib/chat/common/promptSyntax/codecs/base/frontMatterCodec/parsers/frontMatterArray.js -> workbench/contrib/chat/common/promptSyntax/codecs/base/frontMatterCodec/parsers/frontMatterValue.js @aeschli

@jrieken jrieken added important Issue identified as high-priority debt Code quality issues labels Jun 9, 2025
@jrieken jrieken added this to the June 2025 milestone Jun 9, 2025
@connor4312
Copy link
Member

connor4312 commented Jun 9, 2025

Should we push changes to this mega PR or do our own PRs? saw your message

@aeschli
Copy link
Contributor

aeschli commented Jun 9, 2025

#251024 for the frontMatterCodec/parsers cycle

@jrieken
Copy link
Member Author

jrieken commented Jun 10, 2025

Error: CYCLIC dependency: /Users/jrieken/Code/vscode/out/vs/vs/workbench/contrib/mcp/browser/mcpResourceQuickAccess.js -> /Users/jrieken/Code/vscode/out/vs/vs/workbench/contrib/mcp/browser/mcpCommands.js -> /Users/jrieken/Code/vscode/out/vs/vs/workbench/contrib/mcp/browser/mcpResourceQuickAccess.js

@connor4312 I still see this one ⬆️

@jrieken jrieken assigned Tyriar and unassigned lszomoru Jun 10, 2025
@hediet hediet removed their assignment Jun 10, 2025
@Tyriar Tyriar removed their assignment Jun 10, 2025
@jrieken jrieken marked this pull request as ready for review June 11, 2025 06:37
@jrieken jrieken merged commit 73a5bfb into main Jun 11, 2025
8 checks passed
@jrieken jrieken deleted the joh/systematic-mockingbird branch June 11, 2025 07:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debt Code quality issues important Issue identified as high-priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants