-
Notifications
You must be signed in to change notification settings - Fork 8
This PR resolves two critical issues that prevented the typesync application from building and running correctly on Windows environments. These changes ensure a consistent developer experience across all operating systems.
#317
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
This PR resolves two critical issues that prevented the typesync application from building and running correctly on Windows environments. These changes ensure a consistent developer experience across all operating systems.
#317
Conversation
07a823d to
efecdab
Compare
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.
Pull Request Overview
This PR ensures typesync builds and runs on Windows by replacing platform-specific scripts and fixing dynamic import paths.
- Swaps
rm -rffor cross-platformrimrafin the clean script - Introduces
fromFileSystemto convert OS paths tofile://URLs for the migration loader - Updates
Database.tsto use the new loader
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| apps/typesync/package.json | Replaced "rm -rf" with "rimraf" in the clean script and added rimraf to devDependencies |
| apps/typesync/src/Utils.ts | Added fromFileSystem loader that uses pathToFileURL to support Windows migration imports |
| apps/typesync/src/Database.ts | Switched Migrator loader to use the new fromFileSystem function |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (2)
apps/typesync/src/Utils.ts:146
- Add unit tests for the
fromFileSystemloader to cover both POSIX and Windows-style file names and ensure that theimport()URL conversion behaves correctly under each environment.
export const fromFileSystem = (
apps/typesync/src/Utils.ts:139
- Expand the JSDoc for
fromFileSystemto include a description of thedirparameter and the returned loader type, clarifying expected input and behavior.
/**
| Effect.promise(() => | ||
| import( | ||
| /* @vite-ignore */ /* webpackIgnore: true */ | ||
| pathToFileURL(`${dir}/${basename}`).href, |
Copilot
AI
Jul 5, 2025
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.
Use path.join or path.resolve to combine dir and basename when constructing the file path, e.g., pathToFileURL(path.join(dir, basename)).href for clarity and cross-platform safety.
| loader: Migrator.fromFileSystem(fileURLToPath(new URL('migrations', import.meta.url))), | ||
| }).pipe(Layer.provide(SqlLive)); | ||
| loader: fromFileSystem(fileURLToPath(new URL('migrations', import.meta.url))), | ||
| }).pipe(Layer.provide(SqlLive)) |
Copilot
AI
Jul 5, 2025
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.
[nitpick] Add a semicolon at the end of this statement to maintain consistency with the surrounding code style.
| }).pipe(Layer.provide(SqlLive)) | |
| }).pipe(Layer.provide(SqlLive)); |
|
@JoMind great changes, thanks! Can you reapply your commits as signed commits. Due merge rules I can't merge in the organization I can't merge without. If too complicated let me know and I can try to get your commits and re-apply them manually. |
dc1f88b to
efecdab
Compare
Replaces the \ m -rf\ command with \ imraf\ so the clean script works on Windows, macOS, and Linux. Affected files: • apps/typesync/package.json – updated "clean" script
Problem ------- `@effect/sql`’s File-system loader passes a raw Win32 path such as C:\project\apps\typesync\src\migrations\1_create_table__app.js directly to `import()`. On Windows the Node ESM loader interprets the leading “C:” as a URL scheme and throws: Only URLs with a scheme in: file, data, and node are supported … Received protocol 'c:' (Linux and macOS do not throw because the path starts with '/'.) See Node issue: nodejs/node#31710 See Effect issue: Effect-TS/effect#4297 Fix --- 1. Keep `dir` as a *plain OS path* when calling `FileSystem.readDirectory(dir)` (still required by platform-fs). 2. Convert the specifier used by the dynamic `import()` to a **file URL** with `pathToFileURL(...).href`. 3. Extend the filename RegExp to accept both “/” and “\” separators. Result ------ • Behaviour on POSIX remains unchanged. • Migrations now load correctly on Windows. • No other library code is touched; the change can be removed once the upstream patch is released. Affected files: • apps/typesync/src/Database.ts – use fromFileSystem() helper • apps/typesync/src/Utils.ts – add fromFileSystem() helper
efecdab to
9496567
Compare
Sure, signed. |
Key Changes
Use
rimraffor the "clean" scriptrm -rfis not available on Windows.apps/typesync/package.jsonnow usesrimrafso the command works on all platforms.Ship a cross-platform File-system migration loader
Problem – The loader that ships with
@effect/sqlbuilds the specifier forimport()with an OS-specific path (e.g.C:\project\…\1_init.js). On Windows, the Node.js ESM resolver treats the leadingC:as a URL scheme and throws the following error:(See nodejs/node#31710, effect-ts/effect#4297).
Fix – We added
src/Utils.ts::fromFileSystem()which:directoryas a plain OS path when callingFileSystem.readDirectory.import()withpathToFileURL(...).href(e.g.file:///C:/project/…/1_init.js)./and\in the filename RegExp.The loader is plugged into
DatabaseService:Behaviour on Linux/macOS is unchanged; migrations now load on Windows as well.
These two changes eliminate the build error on
cleanand the runtime error, so the project builds and runs successfully on Windows, Linux, and macOS.