Skip to content

chore: make cyclic deps check an error #38543

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.

Sign up for GitHub

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
mohanarpit merged 25 commits into from
Jan 9, 2025
Merged

chore: make cyclic deps check an error #38543

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
086bd6e
chore: make cyclic deps check an error
Jan 8, 2025
370b0e5
add client-check-cyclic-deps as mandatory check for qc-result
Jan 8, 2025
903a8e4
add test cyclic dependency
Jan 8, 2025
fc56f5a
add result check for client-check-cyclic-deps
Jan 8, 2025
d774508
update emoji for Cyclic Dependency Check
Jan 8, 2025
fecf2d4
Update command to count and add comments
Jan 8, 2025
086cc7a
fix comment typo
Jan 8, 2025
116dc9b
Revert "add test cyclic dependency"
Jan 8, 2025
12c0947
install dpdx globally
Jan 8, 2025
b09518e
test changes
Jan 8, 2025
6ea01fa
Revert "test changes"
Jan 8, 2025
ef115da
add file extensions to dpdm glob pattern
Jan 9, 2025
4acb4e2
Add same error emojis as cypress test failure
Jan 9, 2025
8aa14a5
fix typo in step id
Jan 9, 2025
846eed4
add quotes to shell script variables to prevent word splitting
Jan 9, 2025
f4a948b
fix typo in variable
Jan 9, 2025
7d4dece
wrap all shell variables with quotes
Jan 9, 2025
c053aeb
trigger cyclic deps check
Jan 9, 2025
e1e0742
revert: trigger cyclic deps check
Jan 9, 2025
658e605
yarn before cyclic dependency check
Jan 9, 2025
66fd9f8
move install deps to a seperate step and load cached node_modules
Jan 9, 2025
b993677
revert: trigger cyclic deps check
Jan 9, 2025
b4e9182
remove restore cache steps
Jan 9, 2025
57e0bd3
test changes
Jan 9, 2025
49cca4c
revert: trigger cyclic deps check
Jan 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .github/workflows/ci-client-cyclic-deps-check.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,3 +85,8 @@ jobs:
issue_number: prNumber,
body: message
});

# Fail the workflow if cyclic dependencies are found
- name: Fail the workflow if cyclic dependencies are found
if: steps.compare-deps.outputs.has_more_cyclic_deps == 'true' && steps.changed-files.outputs.any_changed == 'true'
run: exit 1
2 changes: 2 additions & 0 deletions .github/workflows/quality-checks.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ jobs:
client-prettier,
client-unit-tests,
client-lint,
client-check-cyclic-deps,
]
if: always()
runs-on: ubuntu-latest
Expand All @@ -116,6 +117,7 @@ jobs:
"${{ needs.client-build.result }}" == "failure" || \
"${{ needs.client-prettier.result }}" == "failure" || \
"${{ needs.client-unit-tests.result }}" == "failure" || \
"${{ needs.client-check-cyclic-deps.result }}" == "failure" || \
"${{ needs.client-lint.result }}" == "failure" ]]; then
echo "Quality checks failed";
exit 1;
Expand Down
8 changes: 8 additions & 0 deletions app/client/src/instrumentation/dummy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import type { Attributes as OTELAttributes } from "@opentelemetry/api";
import type { Span } from "./types";
export type Attributes = OTELAttributes;

@coderabbitai coderabbitai Bot Jan 8, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Break the cyclic dependency with types.ts

There's a circular dependency between this file and types.ts. This file imports Span from ./types while types.ts imports Attributes from here.

Consider moving common types to a separate types file to break this cycle. For example:

- import type { Span } from "./types";
- export type Attributes = OTELAttributes;
+ // Move to a new file: commonTypes.ts
+ export type Attributes = OTELAttributes;
+ export type Span = OTELSpan;

Committable suggestion skipped: line range outside the PR's diff.


export interface DummyType {
attributes: OTELAttributes;
span: Span;
}
11 changes: 4 additions & 7 deletions app/client/src/instrumentation/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,12 @@
import type {
Attributes as OTELAttributes,
TimeInput,
Span as OTELSpan,
} from "@opentelemetry/api";
import type { TimeInput, Span as OTELSpan } from "@opentelemetry/api";
import type { Attributes as DummyAttributes } from "./dummy";

@coderabbitai coderabbitai Bot Jan 8, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Remove circular dependency with dummy.ts

This file imports from ./dummy which creates a circular dependency. This could cause issues with bundling and type resolution.

Move shared types to a common file:

- import type { Attributes as DummyAttributes } from "./dummy";
+ import type { Attributes } from "./commonTypes";

Committable suggestion skipped: line range outside the PR's diff.


export interface WebworkerSpanData {
attributes: OTELAttributes;
attributes: DummyAttributes;
spanName: string;
startTime: TimeInput;
endTime: TimeInput;
}

export type Attributes = OTELAttributes;
export type Span = OTELSpan;
export type Attributes = DummyAttributes;