Skip to content

chore: Added linting to error on relative imports and using path aliases#2506

Merged
jordan-simonovski merged 3 commits into
mainfrom
jordansimonovski/linting-relative-imports
Jun 24, 2026
Merged

chore: Added linting to error on relative imports and using path aliases#2506
jordan-simonovski merged 3 commits into
mainfrom
jordansimonovski/linting-relative-imports

Conversation

@jordan-simonovski

Copy link
Copy Markdown
Contributor

Summary

Based on the conversation in #2502 I've raised a PR to replace relative imports with path aliases.
Linting rules have been updated to catch this in future.

Related PRs: #2502

@changeset-bot

changeset-bot Bot commented Jun 23, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: f93c49c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
@hyperdx/common-utils Patch
@hyperdx/hdx-eval Patch
@hyperdx/api Patch
@hyperdx/app Patch
@hyperdx/cli Patch
@hyperdx/otel-collector Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@greptile-apps

greptile-apps Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Too many files changed for review. (291 files found, 100 file limit)

@vercel

vercel Bot commented Jun 23, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments
Project Deployment Actions Updated (UTC)
hyperdx-oss Ignored Ignored Preview Jun 24, 2026 4:38am
hyperdx-storybook Skipped Skipped Jun 24, 2026 4:38am

Request Review

@github-actions github-actions Bot added the review/tier-4 Critical — deep review + domain expert sign-off label Jun 23, 2026
@github-actions

github-actions Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

🔴 Tier 4 — Critical

Touches auth, data models, config, tasks, OTel pipeline, ClickHouse, or CI/CD.

Why this tier:

  • Critical-path files (2):
    • packages/api/src/tasks/checkAlerts/providers/default.ts
    • packages/api/src/tasks/checkAlerts/providers/index.ts
  • Cross-layer change: touches frontend (packages/app) + backend (packages/api) + shared utils (packages/common-utils)

Review process: Deep review from a domain expert. Synchronous walkthrough may be required.
SLA: Schedule synchronous review within 2 business days.

Stats
  • Production files changed: 120
  • Production lines changed: 593 (+ 686 in test files, excluded from tier calculation)
  • Branch: jordansimonovski/linting-relative-imports
  • Author: jordan-simonovski

To override this classification, remove the review/tier-4 label and apply a different review/tier-* label. Manual overrides are preserved on subsequent pushes.

knudtty
knudtty previously approved these changes Jun 23, 2026

@knudtty knudtty left a comment

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.

🫣

@github-actions

github-actions Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

E2E Test Results

All tests passed • 221 passed • 3 skipped • 1385s

Status Count
✅ Passed 221
❌ Failed 0
⚠️ Flaky 1
⏭️ Skipped 3

Tests ran across 4 shards in parallel.

View full report →

@wrn14897

Copy link
Copy Markdown
Member

@claude review this PR

@github-actions

github-actions Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Claude finished @wrn14897's task in 3m 4s —— View job


PR Review — relative imports → path aliases

  • Read PR context & diff
  • Review ESLint config changes
  • Spot-check import rewrites & critical-path files
  • Post review

This is a large but overwhelmingly mechanical change (../…@/…) plus new no-restricted-imports lint rules across api, app, cli, common-utils, and hdx-eval. Overall it's clean and well-executed. One runtime concern worth verifying before merge, and a couple of minor notes.


🟠 One thing to verify: hdx-eval build won't resolve @/ at runtime

packages/hdx-eval had no @/ aliases before this PR — every import was relative, and it ran via tsx (dev) / jest (tests), both of which resolve tsconfig paths. That's why CI is green.

But its production build is plain tsc with no alias rewriting:

// packages/hdx-eval/package.json
"bin":  "./dist/cli.js",
"main": "./dist/index.js",
"build": "rimraf ./dist && tsc -p tsconfig.build.json"   // ← no tsc-alias / bundler

tsc does not rewrite path aliases in emitted JS, so the built dist/*.js will contain require("@/…") that Node cannot resolve → Cannot find module '@/…' when the bin/main entry is actually run from dist.

Contrast with the other packages, which all stay safe:

  • apitsc && tsc-alias (rewrites aliases) ✅
  • clitsup / bun build (bundler resolves paths) ✅
  • common-utilstsup

Fix: add an alias-rewriting step to hdx-eval's build, e.g. "build": "rimraf ./dist && tsc -p tsconfig.build.json && tsc-alias -p tsconfig.build.json" (add tsc-alias as a devDependency), or switch it to tsup like cli/common-utils. If hdx-eval is only ever run via yarn dev (tsx) and the dist build is unused in practice, this is a non-issue — but the bin/main pointing at dist suggest it's meant to run compiled. Fix this →


🟢 Things done well

  • Rule is correctly scoped. The regex ^\.\.(/|$) only matches parent-relative imports (../, ..); sibling imports (./helpers, ./schemas) are intentionally left relative, which matches the rewrites in the diff. Good call — converting sibling imports to @/ would have been noisy with no benefit.
  • Legitimate exceptions handled properly, not worked around. The two remaining ../ imports both have documented disables:
    • packages/app/src/components/AppNav/AppNav.tsx:44../../../package.json (lives outside src/)
    • packages/app/src/__tests__/TraceRedirectPage.test.tsx:5../../pages/... (Next.js pages/ is outside src/)
  • Alias resolution is wired up everywhere it needs to be — every package has tsconfig paths and a matching jest moduleNameMapper, so tests resolve @/ correctly.
  • e2e override is correctno-restricted-imports: 'off' for tests/e2e/** since those files sit outside src/ and @/ doesn't reach sibling test files.
  • Critical-path rewrites (tasks/checkAlerts/providers/*, mcp/**) are faithful: '..'@/tasks/checkAlerts, '../tools/types'@/mcp/tools/types, etc. No behavioral change.

🔵 Minor / non-blocking

  • The changeset bumps @hyperdx/otel-collector (a Go package) even though no files in packages/otel-collector changed. Harmless, but it's an unnecessary version bump — consider dropping it from the changeset.
  • All bumps are patch; an import-path-only refactor with no user-facing change arguably warrants no changeset at all for the runtime packages, but patch is fine.

Verdict: Looks good to merge once the hdx-eval build (@/ at runtime) is either fixed or confirmed irrelevant. Everything else is solid.
jordansimonovski/linting-relative-imports

@vercel vercel Bot temporarily deployed to Preview – hyperdx-storybook June 24, 2026 04:38 Inactive
@jordan-simonovski jordan-simonovski merged commit 1a64796 into main Jun 24, 2026
19 checks passed
alex-fedotyev added a commit that referenced this pull request Jun 24, 2026
Resolve conflict in NumberTileBackgroundChart.test.tsx: keep the
expanded import set the render and helper tests need, and adopt the
path-alias import (@/components/...) that #2506 now requires in place
of the relative path.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review/tier-4 Critical — deep review + domain expert sign-off

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants