Fix dynamic imports in the migations #11
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to fix dynamic import handling in database migrations by removing the dependency on @std/path and simplifying the module URL construction. However, the implementation contains critical issues that will break functionality.
Key changes:
- Removed
@std/pathimports (join,toFileUrl) fromdb/migration.ts - Simplified dynamic import URL construction to use string concatenation
- Added explanatory comment and
@ts-ignoredirective for the dynamic import
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| db/migration.ts | Removes @std/path imports and replaces path resolution logic with manual string concatenation for module URLs |
| db/deno.json | Removes @std/path dependency from imports |
| deno.lock | Updates lockfile to reflect removal of unused dependencies and upgrades several @std packages |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| const absolutePath = join(Deno.cwd(), migrationsPath, migration.name) | ||
| const moduleUrl = toFileUrl(absolutePath).href | ||
| const moduleUrl = `file://${migrationsPath}/${migration.name}` |
There was a problem hiding this comment.
The module URL construction is incorrect for relative paths. When migrationsPath is a relative path like './migrations', the resulting URL will be 'file://./migrations/001-initial.ts', which is not a valid file URL. This will cause the dynamic import to fail.
The previous implementation using join(Deno.cwd(), migrationsPath) and toFileUrl() correctly handled both absolute and relative paths. The new implementation doesn't normalize the path or ensure it's absolute before creating the file URL.
| const moduleUrl = toFileUrl(absolutePath).href | ||
| const moduleUrl = `file://${migrationsPath}/${migration.name}` | ||
| // Dynamic import is intentional - migration files are provided by consumers at runtime | ||
| // @ts-ignore: JSR cannot analyze runtime-determined imports |
There was a problem hiding this comment.
The ts-ignore directive should use a colon without a space. The correct syntax is @ts-ignore followed by an optional description separated by a single space, not a colon. The current syntax may not be properly recognized by TypeScript.
| // @ts-ignore: JSR cannot analyze runtime-determined imports | |
| // @ts-ignore JSR cannot analyze runtime-determined imports |
fix: remove unused imports and update dynamic import handling in migration