Uh oh!
There was an error while loading. Please reload this page.
Import migrations through a file URL so Windows paths work in Migrator.fromFileSystem - #7166
Conversation
🦋 Changeset detectedLatest commit: 282b9e2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 30 packages
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 |
Bundle Size AnalysisGenerated from PR build output; treat the content below as untrusted.
|
There was a problem hiding this comment.
ℹ️ Two minor observations — the core fix is correct, well-reasoned, and I verified the suite passes.
Reviewed changes
Migrator.fromFileSystem(packages/effect/src/unstable/sql/Migrator.ts): the migration specifier is now resolved through thePathservice (path.join+path.toFileUrl) instead of a bare`${directory}/${basename}`string, so Windows absolute paths become importablefile://URLs. The loader type widens toLoader<FileSystem | Path>, andorDiekeepstoFileUrl's typedBadArgumentfailure a defect soloadMigrationreports it as a normal import error.- Two new loader tests (
packages/effect/test/Migrator.test.ts): a Windows-likePathstand-in provesimportreceivesfile:///C:/migrations/0001_first.js, and a failingtoFileUrlis surfaced as an import error. The existing filesystem test now providesPath.layer. - A
patchchangeset documenting the breaking requirement widening.
I ran npx vitest run --project effect packages/effect/test/Migrator.test.ts → 5 passed, and confirmed the platform NodePath.layer (via node:url.pathToFileURL) plus NodeServices.layer make the fix correct on Windows without breaking aggregate-layer callers. The orDie reasoning checks out against loadMigration's catchDefect. Two rough edges inline.
@v0 or keep the SHA fresh with Dependabot ｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| // `import` needs a file URL: on Windows an absolute path such as | ||
| // `D:\migrations\1_init.ts` is rejected by the ESM loader. `orDie` keeps the | ||
| // failure a defect so `loadMigration` reports it as an import error. | ||
| Effect.flatMap(Effect.orDie(path.toFileUrl(path.join(directory, basename))), (url) => |
There was a problem hiding this comment.
Minor, mostly worth a confirmation: for a relative directory value, this now routes through path.toFileUrl → resolve, so the specifier base silently changes from "resolved against the library module (broken at runtime)" to "resolved against the current working directory" — which is actually consistent with how readDirectory already treats the directory. That's an improvement, but it's a semantic change beyond the Windows fix and isn't called out in the PR description or changeset. If relative-directory (./migrations) support is intended to be CWD-relative, it's fine as-is; otherwise worth a note.
| ) | ||
| // the raw path would have been rejected as protocol "c:" | ||
| assert.include(specifier, "file:///C:/migrations/0001_first.js") |
There was a problem hiding this comment.
Nit: the assertion is coupled to the importer's error-message format. I confirmed plain Node's ERR_MODULE_NOT_FOUND strips the scheme (Cannot find module '/C:/migrations/0001_first.js') — it's only Vitest's wrapper that currently echoes the full file:///... URL, which is why this passes here and fails when reverted. It's a valid regression test today, but a future loader/vitest message change could break it spuriously. Consider asserting on something less format-dependent, e.g. that the defect text contains the normalized forward-slash path without a \ backslash, or pin the message check more loosely.
Uh oh!
There was an error while loading. Please reload this page.
Type
Description
Migrator.fromFileSystempasses the directory and file name straight toimport. On Windows that produces a specifier such asD:\migrations\1_init.ts, which the ESM loader rejects:The loader now builds the specifier with the
Pathservice, which already knows how to produce a file URL for the host platform:Two things worth calling out, since neither is free:
fromFileSystemwidens fromLoader<FileSystem>toLoader<FileSystem | Path>. Core has no WindowsPathimplementation and hardcodingnode:urlhere would be wrong, so the platform has to supply it. Callers on an aggregate layer such asNodeServices.layerare unaffected; callers providingFileSystemalone now also need aPathlayer, and on Windows it has to be a platform-aware one rather than the POSIXPath.layer. The changeset spells this out.toFileUrlfails in the typed error channel withBadArgument, whileloadMigrationonly normalizes defects. WithoutorDiethat failure would escape theMigrationError | SqlErrorchannel thatmakeadvertises, so it is turned back into a defect and reported as an import error like any other.Validation
pnpm checkpnpm lintpnpm vitest run --project effect packages/effect/test/Migrator.test.ts(5 passed)Both new tests were checked against the unfixed code rather than only the fixed code. Reverting the URL conversion fails the first with
expected 'Error: Cannot find module /@id/C:\m…' to include 'file:///C:/migrations/0001_first.js', and droppingorDiefails the second withexpected 'no defect' to include 'defect:'.The Windows case is covered with a stand-in
Pathprovider, because core has no win32 implementation andpackages/effectshould not depend on@effect/platform-node-sharedfor a test.Related
Closes#4297