-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
web: fix client workspaces cicular dependency issue #44192
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,10 +10,8 @@ import { catchError, filter } from 'rxjs/operators' | |
import { HoverMerged } from '@sourcegraph/client-api' | ||
import { DOMFunctions, findPositionsFromEvents, Hoverifier } from '@sourcegraph/codeintellify' | ||
import { asError, ErrorLike, isDefined, isErrorLike, highlightNodeMultiline } from '@sourcegraph/common' | ||
import { HighlightLineRange } from '@sourcegraph/search' | ||
import { ActionItemAction } from '@sourcegraph/shared/src/actions/ActionItem' | ||
import { ViewerId } from '@sourcegraph/shared/src/api/viewerTypes' | ||
import { HighlightResponseFormat } from '@sourcegraph/shared/src/graphql-operations' | ||
import { HoverContext } from '@sourcegraph/shared/src/hover/HoverOverlay.types' | ||
import { Repo } from '@sourcegraph/shared/src/util/url' | ||
import { Icon, Code } from '@sourcegraph/wildcard' | ||
|
@@ -27,15 +25,6 @@ export interface Shape { | |
right?: number | ||
} | ||
|
||
export interface FetchFileParameters { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved
|
||
repoName: string | ||
commitID: string | ||
filePath: string | ||
disableTimeout?: boolean | ||
ranges: HighlightLineRange[] | ||
format?: HighlightResponseFormat | ||
} | ||
|
||
interface Props extends Repo { | ||
commitID: string | ||
filePath: string | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,7 @@ import { HoverMerged } from '@sourcegraph/client-api' | |
import { Hoverifier } from '@sourcegraph/codeintellify' | ||
import { isErrorLike, pluralize } from '@sourcegraph/common' | ||
import { ActionItemAction } from '@sourcegraph/shared/src/actions/ActionItem' | ||
import { FetchFileParameters } from '@sourcegraph/shared/src/backend/file' | ||
import { LineRanking } from '@sourcegraph/shared/src/components/ranking/LineRanking' | ||
import { MatchGroup, MatchItem } from '@sourcegraph/shared/src/components/ranking/PerFileResultRanking' | ||
import { ZoektRanking } from '@sourcegraph/shared/src/components/ranking/ZoektRanking' | ||
|
@@ -26,7 +27,6 @@ import { isSettingsValid, SettingsCascadeProps } from '@sourcegraph/shared/src/s | |
import { TelemetryProps } from '@sourcegraph/shared/src/telemetry/telemetryService' | ||
import { Badge } from '@sourcegraph/wildcard' | ||
|
||
import { FetchFileParameters } from './CodeExcerpt' | ||
import { FileMatchChildren } from './FileMatchChildren' | ||
import { RepoFileLink } from './RepoFileLink' | ||
import { ResultContainerProps, ResultContainer } from './ResultContainer' | ||
|
@@ -125,7 +125,7 @@ export const FileSearchResult: React.FunctionComponent<React.PropsWithChildren<P | |
const contextLinesSetting = | ||
isSettingsValid(props.settingsCascade) && | ||
props.settingsCascade.final && | ||
(props.settingsCascade.final['search.contextLines'] as number | undefined) | ||
props.settingsCascade.final['search.contextLines'] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Auto-fixed by Typescript as a redundant type-casting. |
||
|
||
if (typeof contextLinesSetting === 'number' && contextLinesSetting >= 0) { | ||
return contextLinesSetting | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,5 +16,6 @@ | |
{ "path": "../branded" }, | ||
{ "path": "../http-client" }, | ||
{ "path": "../common" }, | ||
{ "path": "../observability-client" }, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Relying on Typescript project references makes it possible to avoid adding type declarations via the |
||
], | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding of why it's required: the
branded
package hasobservability-client
file in resolved modules, but because it doesn't rely on Typescript project references in some of the edges leading toobservability-client
modules, they are resolved using thistsconfig.json
. The resolved modules graph doesn't have type definitions defined in theobservability-client
packagetsconfig.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 topaths
) because theshared
package is part of multiple circular import graphs, which are not allowed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 aboutobservability-client
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.