web: fix client workspaces cicular dependency issue #44192

valerybugakov · 2022-11-10T08:13:46Z

Context

This PR removes a circular dependency between a couple of client packages to unblock Typescript builds. See more details in the inline comments.

Notes

A list of questions to address later in the documentation/RFC:

This was painful to debug and fix because we have a lot of circular dependencies between packages that do not affect TS builds.
We do not have a 1-1 mapping between Typescript project references and actual imports in source files. E.g., the fact that X doesn't have Y in TS project references doesn't mean X doesn't have imports from Y.
It's unclear why we use paths over typeRoots to add custom type definitions.
In many cases, it's unclear why things should live in package X instead of Y. E.g., why was FetchFileParameters defined in the search-ui package?

TODO

A follow-up PR to ban certain cross-package imports to avoid such problems in the future.
A follow-up documentation/RFC to at least partially address the issues above.

Test plan

The CI is green for this PR.
The CI is green for web: trace search results and search query input #43997. It doesn't have the Typescript build failure as it did without changes introduced by this PR.

App preview:

Web

Check out the client app preview documentation to learn more.

sg-e2e-regression-test-bob · 2022-11-10T08:21:53Z

Bundle size report 📦

Initial size	Total size	Async size	Modules
0.00% (0.00 kb)	0.00% (0.00 kb)	0.00% (0.00 kb)	0.00% (0)

Look at the Statoscope report for a full comparison between the commits 4b2936d and 6579bc4 or learn more.

Open explanation

Initial size is the size of the initial bundle (the one that is loaded when you open the page)
Total size is the size of the initial bundle + all the async loaded chunks
Async size is the size of all the async loaded chunks
Modules is the number of modules in the initial bundle

valerybugakov · 2022-11-11T06:53:00Z

client/search-ui/src/components/CodeExcerpt.tsx

@@ -27,15 +25,6 @@ export interface Shape {
    right?: number
 }

-export interface FetchFileParameters {


Moved FetchFileParameters from the search-ui package to the shared package:

To remove the circular dependency between search-ui and search.

To colocate with the fetch function where this interface is required – fetchHighlightedFileLineRanges.

valerybugakov · 2022-11-11T06:54:37Z

client/search-ui/tsconfig.json

@@ -16,5 +16,6 @@
    { "path": "../branded" },
    { "path": "../http-client" },
    { "path": "../common" },
+    { "path": "../observability-client" },


Relying on Typescript project references makes it possible to avoid adding type declarations via the paths property as we do in some tsocnfig.json files.

valerybugakov · 2022-11-11T07:25:34Z

client/branded/tsconfig.json

@@ -6,7 +6,7 @@
    "sourceRoot": "src",
    "baseUrl": ".",
    "paths": {
-      "*": ["src/types/*", "*"],
+      "*": ["../observability-client/src/types/*", "src/types/*", "*"],


My understanding of why it's required: the branded package has observability-client file in resolved modules, but because it doesn't rely on Typescript project references in some of the edges leading to observability-client modules, they are resolved using this tsconfig.json. The resolved modules graph doesn't have type definitions defined in the observability-client package tsconfig.json, so we need to manually add them here.

Even if we find the resolution path that leads from this package to observability-client modules, it won't be possible to use the Typescript project references (that would remove the need to add type-declarations to paths) because the shared package is part of multiple circular import graphs, which are not allowed.

Even if we find the resolution path that leads from this package to observability-client modules, it won't be possible to use the Typescript project references (that would remove the need to add type-declarations to paths) because the shared package is part of multiple circular import graphs, which are not allowed.

Hm but if we find it and eliminate the resolution path, do we still need to use the TypeScript project references? I’m thinking that ideally the branded module does not know anything about observability-client.

Yes, it should not know anything about it. We have many circular imports between packages, making it hard to analyze this structure and find specific resolution paths. We definitely should do it, but I'd love to have a plan first to ensure we won't regress after fixing the resolution graph.

valerybugakov · 2022-11-11T07:36:43Z

client/search-ui/src/components/FileSearchResult.tsx

-                (props.settingsCascade.final['search.contextLines'] as number | undefined)
+                props.settingsCascade.final['search.contextLines']


Auto-fixed by Typescript as a redundant type-casting.

sourcegraph-bot · 2022-11-11T07:47:15Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff ef37174...4b2936d.

Notify	File(s)
@fkling	client/search-ui/src/components/CodeExcerpt.tsx client/search-ui/src/components/FileMatchChildren.tsx client/search-ui/src/components/FileSearchResult.tsx client/search-ui/src/results/StreamingSearchResultsList.tsx client/search/src/integration/streaming-search-mocks.ts client/web/src/search/results/StreamingSearchResults.tsx
@limitedmage	client/search-ui/src/components/CodeExcerpt.tsx client/search-ui/src/components/FileMatchChildren.tsx client/search-ui/src/components/FileSearchResult.tsx client/search-ui/src/results/StreamingSearchResultsList.tsx client/web/src/search/results/StreamingSearchResults.tsx

sourcegraph-bot · 2022-11-11T07:47:52Z

Codenotify: Notifying subscribers in OWNERS files for diff ef37174...4b2936d.

Notify	File(s)
@sourcegraph/frontend-platform-devs	client/observability-client/src/instrumentations/history.ts client/observability-client/src/types/@opentelemetry/api/index.d.ts client/observability-client/src/types/@opentelemetry/core/index.d.ts client/observability-client/src/types/@opentelemetry/exporter-trace-otlp-http/index.d.ts client/observability-client/src/types/@opentelemetry/instrumentation/index.d.ts client/observability-client/src/types/@opentelemetry/resources/index.d.ts client/observability-client/src/types/@opentelemetry/sdk-trace-base/index.d.ts client/observability-client/src/types/@opentelemetry/semantic-conventions/index.d.ts client/observability-client/tsconfig.json

philipp-spiess

In many cases, it's unclear why things should live in package X instead of Y. E.g., why was FetchFileParameters defined in the search-ui package?

Maybe this is just me but I think our package structure is a bit confusing. E.g I don't know what client/branded is supposed to do that client/web and client/shared can't replicate? 🤔 Also what is the difference between client/shared and client/common.

philipp-spiess · 2022-11-11T14:45:47Z

client/branded/tsconfig.json

@@ -6,7 +6,7 @@
    "sourceRoot": "src",
    "baseUrl": ".",
    "paths": {
-      "*": ["src/types/*", "*"],
+      "*": ["../observability-client/src/types/*", "src/types/*", "*"],


Even if we find the resolution path that leads from this package to observability-client modules, it won't be possible to use the Typescript project references (that would remove the need to add type-declarations to paths) because the shared package is part of multiple circular import graphs, which are not allowed.

Hm but if we find it and eliminate the resolution path, do we still need to use the TypeScript project references? I’m thinking that ideally the branded module does not know anything about observability-client.

valerybugakov · 2022-11-12T03:39:01Z

Maybe this is just me but I think our package structure is a bit confusing

@philipp-spiess, I agree. I'll write a small RFC about that soon, so we can discuss it in the Google doc.

felixfbecker · 2022-11-15T03:21:17Z

Maybe this is just me but I think our package structure is a bit confusing. E.g I don't know what client/branded is supposed to do that client/web and client/shared can't replicate? 🤔 Also what is the difference between client/shared and client/common.

@philipp-spiess have you checked the READMEs for shared and branded? (I was writing an explanation but realized the READMEs already contain it)

client/shared contains code shared between multiple clients (e.g. browser extension and webapp). It needs to work in different environments and adapt to the style of the environment (it can't use/directly depend on our brand design, the wildcard design system, or reference global CSS classes/styles).

client/branded is code shared between clients that are using the brand design. E.g. the options popup in the browser extension reuses some components that are used in the webapp too.

Does that make sense?

philipp-spiess · 2022-11-15T14:00:06Z

@felixfbecker Thanks! This clears things up for me, yeah.

valerybugakov added the team/code-exploration Issues owned by the Code Exploration team label Nov 10, 2022

valerybugakov self-assigned this Nov 10, 2022

cla-bot bot added the cla-signed label Nov 10, 2022

github-actions bot added the team/frontend-platform label Nov 10, 2022

valerybugakov force-pushed the vb/client-workspaces-circular-dep-issue branch from 2630bb0 to 3cd6eda Compare November 10, 2022 09:47

web: fix client workspaces cicular dependency issue

4b2936d

valerybugakov force-pushed the vb/client-workspaces-circular-dep-issue branch from 3cd6eda to 4b2936d Compare November 10, 2022 10:17

valerybugakov commented Nov 11, 2022

View reviewed changes

valerybugakov removed the team/frontend-platform label Nov 11, 2022

valerybugakov marked this pull request as ready for review November 11, 2022 07:45

valerybugakov requested review from vovakulikov, philipp-spiess, taras-yemets and a team November 11, 2022 07:46

philipp-spiess approved these changes Nov 11, 2022

View reviewed changes

limitedmage approved these changes Nov 12, 2022

View reviewed changes

valerybugakov merged commit 16b44cc into main Nov 12, 2022

valerybugakov deleted the vb/client-workspaces-circular-dep-issue branch November 12, 2022 03:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

web: fix client workspaces cicular dependency issue #44192

web: fix client workspaces cicular dependency issue #44192

valerybugakov commented Nov 10, 2022 •

edited

sg-e2e-regression-test-bob commented Nov 10, 2022 •

edited

valerybugakov Nov 11, 2022 •

edited

valerybugakov Nov 11, 2022

valerybugakov Nov 11, 2022

philipp-spiess Nov 11, 2022

valerybugakov Nov 12, 2022

valerybugakov Nov 11, 2022

sourcegraph-bot commented Nov 11, 2022

sourcegraph-bot commented Nov 11, 2022

philipp-spiess left a comment •

edited

philipp-spiess Nov 11, 2022

valerybugakov commented Nov 12, 2022 •

edited

felixfbecker commented Nov 15, 2022

philipp-spiess commented Nov 15, 2022

		(props.settingsCascade.final['search.contextLines'] as number \| undefined)
		props.settingsCascade.final['search.contextLines']

web: fix client workspaces cicular dependency issue #44192

web: fix client workspaces cicular dependency issue #44192

Conversation

valerybugakov commented Nov 10, 2022 • edited

Context

Notes

TODO

Test plan

App preview:

sg-e2e-regression-test-bob commented Nov 10, 2022 • edited

Bundle size report 📦

valerybugakov Nov 11, 2022 • edited

Choose a reason for hiding this comment

valerybugakov Nov 11, 2022

Choose a reason for hiding this comment

valerybugakov Nov 11, 2022

Choose a reason for hiding this comment

philipp-spiess Nov 11, 2022

Choose a reason for hiding this comment

valerybugakov Nov 12, 2022

Choose a reason for hiding this comment

valerybugakov Nov 11, 2022

Choose a reason for hiding this comment

sourcegraph-bot commented Nov 11, 2022

sourcegraph-bot commented Nov 11, 2022

philipp-spiess left a comment • edited

Choose a reason for hiding this comment

philipp-spiess Nov 11, 2022

Choose a reason for hiding this comment

valerybugakov commented Nov 12, 2022 • edited

felixfbecker commented Nov 15, 2022

philipp-spiess commented Nov 15, 2022

valerybugakov commented Nov 10, 2022 •

edited

sg-e2e-regression-test-bob commented Nov 10, 2022 •

edited

valerybugakov Nov 11, 2022 •

edited

philipp-spiess left a comment •

edited

valerybugakov commented Nov 12, 2022 •

edited