Conversation
|
Reviewer assigned: @hiaaryan You are the primary reviewer for this PR. Please complete your review within 24 hours. |
|
@AMar4enko This PR has 1 guardrail violation: 1. Tests RequiredNo test files modified. Please add tests or use the no-tests-needed label. Fix the issues above and push again. Add |
|
Reviewer assigned: @francisco-m001 You are the primary reviewer for this PR. Please complete your review within 24 hours. |
There was a problem hiding this comment.
Pull request overview
This PR ports upstream improvements to the Effect Schema → Drizzle integration, primarily enabling composable field(...) annotations (accepting either a Drizzle column builder or a transformation), and fixing NullOr propagation in both types and runtime compilation. It also rewires internal annotations/types and adds new tooling/config for type tests (tstyche) and linting (oxlint).
Changes:
- Add composable Drizzle
field(...)/drizzleType(...)support and rewire annotation IDs (ColumnId,TableId, etc.). - Fix
NullOrpropagation so nullability is preserved/overridden correctly through composition. - Add/expand test coverage (type tests + runtime snapshot tests) and introduce oxlint + tstyche config.
Reviewed changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tstyche.config.json | Adds tstyche configuration for type-level tests. |
| tsdown.config.ts | Removes previous tsdown build configuration. |
| src/types.ts | Updates ColumnBuilderForField / TableForSchema typing to support new annotations and better nullability behavior. |
| src/model.ts | Reworks schemaDrizzle internals; introduces drizzleType (aliased as field) composition logic. |
| src/compiler.ts | Updates compiler to apply column annotations and preserve/override notNull correctly during transformations. |
| src/annotations.ts | Replaces old annotation symbols with ColumnId/TableId/etc and new accessors. |
| src/pg/index.ts | Adjusts pg adapter behavior to ensure default notNull semantics and correct nullable handling. |
| src/const.ts | Updates package namespace constant for annotation symbols. |
| src/adapter.ts | Updates schema typing imports and lint directives. |
| src/tests/types.tst.ts | Expands type tests for field composition, NullOr typing, branded schemas, and Generated. |
| src/tests/annotations.tst.ts | Updates type assertion to new ColumnId symbol-based annotation. |
| src/tests/makeBaseModel.tst.ts | Updates table-name annotation usage to TableNameId. |
| src/tests/makeBaseModel.test.ts | Same as above for runtime tests. |
| src/tests/support.ts | Adds oxlint directive for consistent type imports. |
| src/tests/pg/fixtures.ts | Updates fixtures to new field usage and exports additional helpers. |
| src/tests/pg/compiler.spec.ts | Adds coverage for type-preserving NullOr with explicit column builders. |
| src/tests/pg/test.tst.ts | Adds type test for jsonb + NullOr composition. |
| src/tests/pg/test.spec.ts | Adds runtime tests for field composition and NullOr notNull override; adds snapshots. |
| src/tests/pg/full.spec.ts | Enables previously skipped snapshot-style “real-world example” compilation test. |
| src/tests/pg/snapshots/test.spec.ts.snap | Adds vitest snapshot output for runtime compilation tests. |
| src/tests/pg/snapshots/full.spec.ts.snap | Adds snapshot output for the “full” compilation test. |
| package.json | Switches build/test scripts and adds oxlint + updated tooling deps. |
| pnpm-lock.yaml | Updates lockfile for new tooling dependencies. |
| .oxlintrc.jsonc | Adds oxlint configuration and plugin setup. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { type Adapter as AdapterNs } from "./adapter"; | ||
| import type { Model } from "@effect/sql"; | ||
|
|
||
| export { Schema } |
There was a problem hiding this comment.
Schema is imported with import type, but then re-exported as a value (export { Schema }). With isolatedModules: true, this will fail to compile; re-export it as a type-only export (or change the import to a value import if you truly need a runtime export).
| export { Schema } | |
| export type { Schema } |
| > = S extends Annotations.AnyWithAnnotation<WithTableFactory, infer C> | ||
| ? C | ||
| : Adapter.BuildTable< | ||
| > = S extends {[TableId]: infer T} |
There was a problem hiding this comment.
TableForSchema checks S extends { [TableId]: infer T }, but TableId is added as an optional property in Annotations.Table (readonly [TableId]?: ...). Because an optional property does not satisfy a required one, this conditional will never match and annotated tables will be ignored. Change the conditional to match an optional TableId property (e.g. S extends { readonly [TableId]?: infer T }).
| > = S extends {[TableId]: infer T} | |
| > = S extends { readonly [TableId]?: infer T } |
|
|
||
| return stored; |
There was a problem hiding this comment.
There is unreachable code in the column transformation rule: after returning from the if/else that handles notNull, there is another return stored; that can never execute. This makes the control flow harder to follow; remove the dead return (and the now-unneeded blank line) to keep the rule clear.
| return stored; |
| export const getTableAnnotation: ( | ||
| ast: SchemaAST.AST, | ||
| ) => Option<DrizzleTable> = SchemaAST.getAnnotation<DrizzleTable>(TableId); |
There was a problem hiding this comment.
TableId is used to store the table factory in schemaDrizzle.table(...) (see src/model.ts), but getTableAnnotation is typed as returning a DrizzleTable. This is a type mismatch that will mislead callers and can hide runtime errors (they'll receive a function, not a table). Update getTableAnnotation (and its generic type argument) to reflect the actual annotation type stored under TableId (the table factory).
package.json
Outdated
| "prepublishOnly": "pnpm run build && pnpm run test && pnpm run test:types" | ||
| "test": "vitest run && tstyche", | ||
| "typecheck": "tsgo --noEmit", | ||
| "build": "tsc", |
There was a problem hiding this comment.
build runs tsc, but typescript is not listed in devDependencies (and tsc is not provided by @typescript/native-preview, which provides tsgo). This will make pnpm build fail in a clean install; add typescript as a devDependency or change the build script to use the installed compiler binary you intend (e.g. tsgo).
| "build": "tsc", | |
| "build": "tsgo", |
.oxlintrc.jsonc
Outdated
| "drizzle/enforce-delete-with-where": "error", | ||
| "react/jsx-pascal-case": "error", | ||
| }, | ||
| "globals": { | ||
| "node": "readonly", | ||
| "es2021": "readonly" | ||
| }, | ||
| "jsPlugins": [ | ||
| "@sellhub/oxlint-plugin", | ||
| { | ||
| "name": "import-js", | ||
| "specifier": "eslint-plugin-import" | ||
| }, | ||
| "eslint-plugin-drizzle" | ||
| ] |
There was a problem hiding this comment.
The oxlint config enables JS plugins (@sellhub/oxlint-plugin, eslint-plugin-import, eslint-plugin-drizzle), but none of these packages are declared in this repo's dependencies/devDependencies. Unless they are provided by the workspace root, pnpm lint will fail at runtime when oxlint tries to load them; add the missing plugin packages or remove them from jsPlugins.
| "drizzle/enforce-delete-with-where": "error", | |
| "react/jsx-pascal-case": "error", | |
| }, | |
| "globals": { | |
| "node": "readonly", | |
| "es2021": "readonly" | |
| }, | |
| "jsPlugins": [ | |
| "@sellhub/oxlint-plugin", | |
| { | |
| "name": "import-js", | |
| "specifier": "eslint-plugin-import" | |
| }, | |
| "eslint-plugin-drizzle" | |
| ] | |
| "react/jsx-pascal-case": "error", | |
| }, | |
| "globals": { | |
| "node": "readonly", | |
| "es2021": "readonly" | |
| } |
Summary
fieldcan be given drizzle column definition directlyallows for more granular composition
NullOr now properly propagates in types and runtime. By default all columns are notNull. To make column nullable wrap schema with NullOr or OptionalFromNull.
Important - order of composition matters. Drizzle field annotations must be added on the lowest level.
Context / Linked Task
Scope
Task Plan
Tests
Test notes:
pnpm testpnpm test:e2eScreenshots (frontend-only)
Before:
(attach image / gif)
After:
(attach image / gif)
Checklist
console.logordebuggerleft in code