feat: effect 4 rewrite - #7
Conversation
|
Warning Review limit reached
Next review available in: 21 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe package is rewritten for Effect 4 and ESM-only publication. It adds validated changesets, changelogs, stored values, evolution, versioned schemas, typed errors, tests, documentation, package smoke tests, and updated CI and release workflows. ChangesEffect 4 evolution API
Packaging and project operations
Documentation and migration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Consumer
participant evolve
participant ChangesetAdapters
participant VersionedSchema
Consumer->>evolve: Provide stored input
evolve->>ChangesetAdapters: Apply pending changesets
ChangesetAdapters-->>evolve: Return versioned stored value
evolve->>VersionedSchema: Decode evolved value
VersionedSchema-->>Consumer: Return application value
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
package.json (1)
35-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLint Node scripts with Node globals.
Add
scriptsto both lint commands. Add ascripts/**/*.mjsESLint block withglobals.node. Limitglobals.vitestto test files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package.json` around lines 35 - 36, Update package.json scripts "fix:lint" and "lint" to include the scripts directory. In eslint.config.mjs, add a scripts/**/*.mjs configuration block with globals.node, and restrict globals.vitest to test-file patterns only. Affected sites: package.json lines 35-36 require both command updates; eslint.config.mjs line 34 requires the ESLint glob and global-scope changes.Source: Linters/SAST tools
src/VersionedSchema.ts (2)
10-13: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueThe encoded type is wider than the runtime schema.
VersionedEncodeddeclares_version: number, but the stored struct usesSchema.Literal(latest). A caller can build an encoded value with any version number and satisfy the type, then fail at decode time. If this widening is deliberate for stored-protocol compatibility, document the reason next toVersionedEncoded.Also applies to: 48-51
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/VersionedSchema.ts` around lines 10 - 13, Align VersionedEncoded with the runtime schema by constraining _version to the accepted Schema.Literal version type instead of any number, so encoded values cannot claim unsupported versions; if broad numeric compatibility is intentional, document that rationale directly beside VersionedEncoded.
66-69: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
evolveAndDecodefails obscurely if the metadata symbol is missing.
Object.definePropertyattaches the changelog to one specific schema instance. Any later transformation or a cast produces a schema without the symbol.schema[versionedSchemaMetadata]is thenundefined, andevolvecallslatestVersion(undefined), which throws aTypeErroroutside the Effect error channel. Add an explicit check with a clear message.🛡️ Proposed guard
): Effect.Effect< Schema.Struct.Type<Fields>, EvolutionError | Schema.SchemaError, R | Schema.Struct.DecodingServices<Fields> - > => - evolve( - schema[versionedSchemaMetadata], - options, - )(input).pipe(Effect.flatMap(Schema.decodeUnknownEffect(schema))); + > => { + const changelog = schema[versionedSchemaMetadata]; + if (changelog === undefined) { + throw new TypeError( + "evolveAndDecode requires a schema created with versioned(changelog).", + ); + } + return evolve(changelog, options)(input).pipe( + Effect.flatMap(Schema.decodeUnknownEffect(schema)), + ); + };Also applies to: 85-88
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/VersionedSchema.ts` around lines 66 - 69, Add an explicit guard in evolveAndDecode before using schema[versionedSchemaMetadata], and likewise in the corresponding path around the later referenced lines, to detect missing metadata and fail with a clear message through the existing Effect error channel. Ensure latestVersion is never called with undefined, while preserving normal behavior for schemas carrying the metadata.src/StoredValue.ts (1)
59-63: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueArray elements skip the data-property check that object properties receive.
The object branch verifies each own key is an enumerable data property through
Object.getOwnPropertyDescriptor. The array branch reads elements withvalue.every(...), which invokes accessors. An array with an index defined as a getter therefore passes validation, but it is not a plain JSON array. Consider applying the same descriptor check to array indexes.♻️ Proposed alignment of the array branch
if (Array.isArray(value)) { valid = Object.getPrototypeOf(value) === Array.prototype && hasOnlyArrayIndexes(value) && - value.every((item) => isJsonValueAt(item, ancestors)); + value.every((_item, index) => { + const descriptor = Object.getOwnPropertyDescriptor( + value, + index, + ); + return ( + descriptor !== undefined && + "value" in descriptor && + descriptor.enumerable && + isJsonValueAt(descriptor.value, ancestors) + ); + }); } else {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/StoredValue.ts` around lines 59 - 63, Update the array validation branch in isJsonValueAt to inspect each array index with its own property descriptor before validating the element, matching the object-property data-descriptor check and rejecting accessor-based indexes. Replace the value.every traversal with descriptor-based validation while preserving the existing prototype, index-only, and recursive isJsonValueAt checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 23-25: Add a workflow-level permissions configuration in the CI
workflow granting only read access to repository contents, such as contents:
read, so the typecheck, coverage, and smoke-package jobs cannot use a
write-capable GITHUB_TOKEN.
In `@CONTEXT.md`:
- Around line 7-8: Update the “Stored value” definition in CONTEXT.md to state
that _version is an own property whose value is a non-negative safe integer,
matching the guarantees documented in README.md and
docs/plans/effect-v4-rewrite.md.
In `@docs/adr/0005-reserve-the-version-marker.md`:
- Around line 1-3: Update the ADR’s root-marker contract to reserve only the
root-level _version property, explicitly allow nested business properties named
_version, and include test among the prohibited JSON Patch operations targeting
the root marker. Align the wording with the implementation and migration guide.
In `@docs/adr/0007-release-v2-without-a-prerelease-line.md`:
- Around line 1-3: Update the release documentation around the v2 baseline to
require creating the missing Git tag v1.0.3 on the commit that produced package
version 1.0.3 before publishing, ensuring semantic-release calculates the
breaking change as stable 2.0.0.
In `@docs/migration-v2.md`:
- Around line 27-35: Update the legacy example around Configuration and
createChangelog to define the v1 changeset before it is passed to
createChangelog, using the documented changeset declaration from the migration
API. Keep the example copy-pastable and leave the subsequent changelog,
versioned configuration, and decode flow unchanged.
In `@src/Changelog.ts`:
- Around line 83-85: Update src/Changelog.ts lines 83-85 in createChangelog to
defensively copy and recursively freeze the normalized changesets, including
each changeset’s _version and JSON Patch operations, before branding and
returning ValidChangelog. Add tests in test/Changelog.spec.ts lines 29-43
confirming mutations to the original changeset or patch after construction do
not alter the validated changelog.
- Around line 19-25: Update ownsRootVersionMarker and its use in
violatesMarkerOwnership so the empty JSON Pointer "" is treated as a
reserved-marker ownership violation for both operation.path and operation.from,
while preserving existing /_version handling. Add root-pointer test cases in
test/Changelog.spec.ts lines 95-110 covering path and from operations.
In `@test/StoredValue.spec.ts`:
- Around line 26-31: Update the parameter cases in the non-object-root test
around the it.each call so every input is wrapped in a single-element tuple,
including the empty and populated array roots. Preserve the existing rejects a
non-object root assertion while ensuring decodes receives each complete root
value.
---
Nitpick comments:
In `@package.json`:
- Around line 35-36: Update package.json scripts "fix:lint" and "lint" to
include the scripts directory. In eslint.config.mjs, add a scripts/**/*.mjs
configuration block with globals.node, and restrict globals.vitest to test-file
patterns only. Affected sites: package.json lines 35-36 require both command
updates; eslint.config.mjs line 34 requires the ESLint glob and global-scope
changes.
In `@src/StoredValue.ts`:
- Around line 59-63: Update the array validation branch in isJsonValueAt to
inspect each array index with its own property descriptor before validating the
element, matching the object-property data-descriptor check and rejecting
accessor-based indexes. Replace the value.every traversal with descriptor-based
validation while preserving the existing prototype, index-only, and recursive
isJsonValueAt checks.
In `@src/VersionedSchema.ts`:
- Around line 10-13: Align VersionedEncoded with the runtime schema by
constraining _version to the accepted Schema.Literal version type instead of any
number, so encoded values cannot claim unsupported versions; if broad numeric
compatibility is intentional, document that rationale directly beside
VersionedEncoded.
- Around line 66-69: Add an explicit guard in evolveAndDecode before using
schema[versionedSchemaMetadata], and likewise in the corresponding path around
the later referenced lines, to detect missing metadata and fail with a clear
message through the existing Effect error channel. Ensure latestVersion is never
called with undefined, while preserving normal behavior for schemas carrying the
metadata.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d21fdd15-d7d3-4ef9-871d-c9c6aada1a48
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (50)
.github/workflows/ci.yml.github/workflows/release.yml.releasercCONTEXT.mdREADME.mddocs/adr/0001-target-effect-4-exclusively.mddocs/adr/0002-preserve-the-stored-value-protocol.mddocs/adr/0003-retain-source-defined-immutability-helper-changesets.mddocs/adr/0004-separate-evolution-from-business-schema-decoding.mddocs/adr/0005-reserve-the-version-marker.mddocs/adr/0006-publish-esm-only.mddocs/adr/0007-release-v2-without-a-prerelease-line.mddocs/adr/0008-derive-schema-version-from-the-changelog.mddocs/adr/0009-advance-the-version-marker-after-each-changeset.mddocs/migration-v2.mddocs/plans/effect-v4-rewrite.mdeslint.config.mjsjest.config.jspackage.jsonscripts/build.mjsscripts/package-smoke.mjssrc/Changelog.tssrc/Changeset.tssrc/Evolution.tssrc/EvolutionError.tssrc/StoredValue.tssrc/VersionedSchema.tssrc/api/Changeset.tssrc/api/EvolutionError.tssrc/api/index.tssrc/core/Evolutions.tssrc/core/index.tssrc/index.tstest/Changelog.spec.tstest/Compatibility.spec.tstest/Evolution.spec.tstest/EvolutionError.spec.tstest/StoredValue.spec.tstest/VersionedSchema.spec.tstest/core/Evolutions.spec.tstest/core/__fixtures__/configuration/v0.tstest/core/__fixtures__/configuration/v1.tstest/core/__fixtures__/configuration/v2.tstest/fixtures/configuration.tstest/type-contracts.tstsconfig.eslint.jsontsconfig.jsontsconfig.module.jsontsconfig.test.jsonvitest.config.ts
💤 Files with no reviewable changes (11)
- tsconfig.module.json
- test/core/fixtures/configuration/v0.ts
- src/core/index.ts
- test/core/Evolutions.spec.ts
- jest.config.js
- src/api/Changeset.ts
- src/core/Evolutions.ts
- test/core/fixtures/configuration/v2.ts
- test/core/fixtures/configuration/v1.ts
- src/api/EvolutionError.ts
- src/api/index.ts
813d25f to
629088f
Compare
BREAKING CHANGE: replace the io-ts/fp-ts and CommonJS APIs with an Effect 4-native ESM interface while preserving the stored-value protocol.
629088f to
dec7586
Compare
|
🎉 This PR is included in version 3.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
1 participant
Summary by CodeRabbit
New Features
Documentation
Packaging