-
-
Notifications
You must be signed in to change notification settings - Fork 35
Dynamic ruleName for migration
#2987
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
Conversation
WalkthroughTests and plugin code were updated to use a dynamically computed ESLint rule key based on TSDOWN_VERSION. The test now stubs the env and dynamically imports the rule module. The plugin exports rules with a computed key. The build config defines the env variable from package.json. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Tester as Test Runner (vitest)
participant Env as vi.stubEnv
participant Mod as migration/index.ts
participant ESLint as RuleTester
Tester->>Env: stub process.env.TSDOWN_VERSION = manifest.version
note over Env: Ensures version available at import time
Tester->>Mod: dynamic import()
Mod->>Mod: compute ruleName from TSDOWN_VERSION (major)
Mod->>Tester: export rules { [ruleName]: theRule }
Tester->>ESLint: tester.run(ruleName, theRule, cases)
ESLint-->>Tester: results (valid/invalid as specified)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
migration/index.spec.ts (1)
13-17: Refactor async setup tobeforeAllinstead of asyncdescribe.Async
describecallbacks are discouraged in testing frameworks. The dynamic import and environment setup should occur inbeforeAllto follow best practices.Apply this diff to refactor:
-describe("Migration", async () => { - vi.stubEnv("TSDOWN_VERSION", manifest.version); - const { default: migration } = await import("./index.ts"); - const ruleName = `v${manifest.version.split(".")[0]}`; - const theRule = migration.rules[ruleName as keyof typeof migration.rules]; +describe("Migration", () => { + let migration: Awaited<ReturnType<typeof import("./index.ts")>>["default"]; + let ruleName: string; + let theRule: ReturnType<typeof import("./index.ts")>["default"]["rules"][string]; + + beforeAll(async () => { + vi.stubEnv("TSDOWN_VERSION", manifest.version); + const imported = await import("./index.ts"); + migration = imported.default; + ruleName = `v${manifest.version.split(".")[0]}`; + theRule = migration.rules[ruleName as keyof typeof migration.rules]; + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
migration/index.spec.ts(1 hunks)migration/index.ts(2 hunks)migration/tsdown.config.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-29T06:00:10.793Z
Learnt from: RobinTail
PR: RobinTail/express-zod-api#2974
File: express-zod-api/src/zts-helpers.ts:2-3
Timestamp: 2025-09-29T06:00:10.793Z
Learning: tsdown handles module resolution and bundling independently of TypeScript's native compilation settings like rewriteRelativeImportExtensions, making such TypeScript emit-related configuration unnecessary when using tsdown for builds.
Applied to files:
migration/tsdown.config.ts
🔇 Additional comments (2)
migration/tsdown.config.ts (1)
2-2: LGTM! Clean manifest integration.The import attributes syntax and define block are correctly structured. The double-quoted string interpolation ensures tsdown replaces
process.env.TSDOWN_VERSIONwith a proper string literal at build time.Also applies to: 9-11
migration/index.ts (1)
96-96: Dynamic rule export key format validatedruleNamederives frommanifest.version.split('.')[0]andmigration/package.jsonversion matches semver, ensuring validv<number>keys.
Need it for #2980
Summary by CodeRabbit
New Features
Chores
Tests