⬆️ Upgrade effection to 4.1.0 - #224
Conversation
|
Warning Review limit reached
Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe changes update Effection imports to version 4, add typed access to the runtime iterator in ChangesEffection v4 compatibility
Estimated code review effort: 1 (Trivial) | ~5 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
commit: |
04e5636 to
902cebb
Compare
The lockfile was pinned at effection 4.0.2 while every package already declared `effection: "^4"`, so the workspace never picked up 4.1. Refresh the lockfile so all 28 workspace projects resolve 4.1.0. Two things surfaced once the resolution moved: - `@effectionx/inline` reads `Coroutine["data"].iterator` — swapping that iterator is the whole mechanism of the package. It is still there at runtime, but it was dropped from Effection's public `Coroutine` type in 4.0.3, so the type check failed. Restate the field locally. - `inline` imports effection in `mod.ts` and in its tests but had no devDependency on it, and was resolving through root hoisting. Declare it. `deno-deploy/deno.json` was the last thing bound to effection 3; its import map now points at ^4. The package is deprecated and excluded from the pnpm workspace, so nothing else about it changes.
`inline` splices an operation into the current frame by swapping the coroutine's iterator. It did that by redefining `iterator` on `routine.data` with a getter. That stopped working. Up to 4.0.2 the reducer drove the routine by reading `routine.data.iterator` through its getter, so shadowing the property was enough. Since the task/coroutine lifecycle unification, `Coroutine.step()` reads a closure variable that only `data`'s setter writes, and never consults the property — so the override was silently ignored and the routine kept driving the original iterator. Assign through the setter instead. Restores `inline`, `esbuild` and `swc` node tests, which were failing on 4.1.0 with undefined/NaN results.
Both comments opened by narrating what the code does. Keep only what a reader cannot infer: the field is absent from the published types, and assigning is not interchangeable with defining the property.
The type comment gave the version the field was dropped in; what matters is that the omission is deliberate because it's a private API. The assignment comment described the mechanism; lead with the instruction, since the failure it prevents is silent.
902cebb to
5e145e9
Compare
Why
Every package already declared
effection: "^4", but the lockfile was pinned at 4.0.2, so the workspace never actually picked up 4.1. This refreshes the lockfile so all 28 workspace projects resolve 4.1.0.What surfaced
Moving off 4.0.2 broke
@effectionx/inlinein two separate ways. Both are fixed here.1.
inlinewas silently a no-op on 4.1 (runtime bug)inlinesplices an operation into the current frame by swapping the coroutine's iterator, which it did by redefiningiteratoronroutine.datawith a getter.Up to 4.0.2 the reducer drove the routine by reading
routine.data.iteratorthrough that getter, so shadowing the property worked. Since the task/coroutine lifecycle unification,Coroutine.step()reads a closure variable that onlydata's setter writes, and never consults the property:So the override was ignored and the routine kept driving the original iterator.
inlinedid nothing. Fixed by assigning through the setter (data.iterator = ...) instead of redefining the property.This was a real test failure, not a theoretical one —
inline.test.tswas returningundefined/NaNwhere it expected values.2.
Coroutine["data"].iteratoris no longer in the public typesIt was dropped from the published
Coroutinetype in 4.0.3 — not 4.1 — and the repo never noticed because the lockfile hadn't moved.mod.tsnow restates the field locally.3.
inlinehad no devDependency on effectionDespite importing it in
mod.ts,inline.test.ts,esbuild.test.tsandswc.test.ts. It was resolving through root hoisting. Now declared, matching every other package.deno-deploy/deno.jsonwas the last thing in the repo bound to effection 3 (npm:effection@^3→^4). That package is deprecated and excluded from the pnpm workspace, so nothing else about it changes.Version bumps
@effectionx/inline0.0.1→0.0.2.Verification
Run locally with the SWC wasm plugin built (
pnpm build), matching CI:pnpm install --frozen-lockfile— clean; all 28 projects resolveeffection@4.1.0pnpm check,pnpm check:tsrefs,pnpm lint,pnpm fmt:check— cleanpnpm test:node— 34 passed, 0 failed (was failing before theinlinefix)pnpm test— 381 passed, 6 skippedpnpm test:matrix— all 10 combinations pass, including effection 3.0.0 and 4.1.0Follow-ups (separate PRs)
First of three. Next: a policy documenting that async teardown must use
ensure()rather than afinallyblock, then the migration of the 8 sites in this repo that currentlyyield*insidefinally.