✨ Add inline() operation to collapse nested yield* delegation#117
✨ Add inline() operation to collapse nested yield* delegation#117
Conversation
|
Did you decide to rename |
|
@taras its "star" optimization, as in ⭐ The reason is that it is an optimization of the The idea is that we can roll up any number of levels of recursion into a single yield point by transforming: yield* operation;into yield star(operation); |
|
Is the readme unrelated? |
2c40e5d to
d13cb1e
Compare
|
Yeah, I just copied it over from chain. I was working on this, but wanted to push up my work so I wouldn't lose it. I've updated the readme. Note that this won't pass the tests because it is |
d13cb1e to
eb74f8f
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
eb74f8f to
c4d59e3
Compare
commit: |
4531f9c to
3fb48a7
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@inline/mod.ts`:
- Around line 4-24: The cast Ok() as Result<T> in inline<T>(operation:
Operation<T>) bypasses type safety (Ok() returns Result<void>), so add a brief
JSDoc above the inline function (or an inline comment next to the cast) stating
that the cast is intentional for this inlining optimization, describing the
trade-off (consumers must cast/handle unknown return types) and referencing the
symbols involved (inline, Ok(), Result<T>, InlineIterator, isInlineIterator,
routine.data.iterator) so future maintainers understand why the type assertion
is required and do not remove it accidentally.
- Around line 30-32: The type guard isInline currently uses a fragile
`"operation" in effect` check which may misclassify other objects; change it to
test for the explicit brand used by isInlineIterator (e.g. check that (effect as
any)["@effectionx/inline"] === true) and update the inline() factory to set that
brand on returned Inline instances (set the same "@effectionx/inline" property
to true) so the guard reliably distinguishes Inline<T> from other Effect<T>
values; keep references to isInline, Inline, Effect, isInlineIterator and
inline() when locating the changes.
In `@inline/package.json`:
- Around line 1-33: The package.json is missing the test runtime dependency and
package publishing settings: add "expect" under devDependencies (to match the
import in inline/star.test.ts) and add a "files" array including "dist" and
"mod.ts" so only intended files are published; update the "devDependencies"
object to include "expect" and add a top-level "files" property with
["dist","mod.ts"] while keeping existing fields like "exports", "main", and
"types" unchanged.
In `@inline/README.md`:
- Around line 1-16: README title and content are inaccurate and underspecified
for publishing: update the title to reference the package name
`@effectionx/inline` and the exported function inline(), replace all occurrences
of star() with inline(), add missing sections (Installation with npm install
`@effectionx/inline`, Usage with concrete examples showing yield* operation ->
yield inline(operation), API reference describing inline(operation) behavior and
return type implications, and a Trade-offs section listing loss of type safety
(unknown return), lost generator stack traces, and intended hot-path use), and
fix Markdown formatting by adding blank lines before and after all fenced code
blocks to satisfy markdownlint.
In `@inline/star.test.ts`:
- Around line 98-102: The helper function constant currently omits an explicit
return type; update its signature to declare the return type as Operation<T>
(e.g., function constant<T>(value: T): Operation<T>) and ensure the returned
object implements [Symbol.iterator] correctly (matching the stateless-streams
pattern used elsewhere) so the type and iterator contract are explicit and
consistent with other uses of Operation<T>.
In `@inline/tsconfig.json`:
- Around line 9-13: The tsconfig.json "references" array incorrectly includes
the workspace package reference to "../bdd" (i.e. `@effectionx/bdd`) even though
inline/mod.ts only imports from the effection peer dependency; remove the
"../bdd" entry so "references" becomes an empty array, save the tsconfig, and
run pnpm check to verify TypeScript compilation passes.
f5f9113 to
1e23cd8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@inline/mod.ts`:
- Around line 62-91: InlineIterator.return currently discards a delegated
iterator's returned value; when calling this.current.return(...) or
top.return(...), if the call completes with {done: true, value: r} you must
propagate r outward instead of keeping the original input value. Modify
InlineIterator.return so that after a successful call to this.current.return or
top.return, if result.done is true set value = result.value and continue
unwinding the stack (and if result.done is false return result immediately);
keep the existing try/catch and this.raise(error) behavior. Ensure the final
returned object uses the last propagated value.
In `@inline/README.md`:
- Around line 8-12: The sentence "The reason is because when javascript composes
generators via `yield*`, each level of nesting creates an intermediate generator
frame." is wordy and redundant; edit the sentence that contains "The reason is
because when javascript composes generators via `yield*`" to a tighter phrasing
such as "When JavaScript composes generators with `yield*`, each nesting level
creates an intermediate generator frame," removing "The reason is because" and
fixing capitalization of "JavaScript" for clarity and readability.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@inline/inline.test.ts`:
- Around line 98-102: The constant helper currently returns a manual iterator
object; replace it with the generator-based pattern by implementing
constant<T>(value: T): Operation<T> using a generator method for
[Symbol.iterator] (i.e. *[Symbol.iterator]()) that yields the provided value and
then completes; update the function signature and ensure it still satisfies the
Operation<T> type and iterator protocol (referencing the constant function and
the Operation<T> type to locate the implementation).
In `@pnpm-workspace.yaml`:
- Line 17: The package entry "inline" is out of alphabetical order in the pnpm
workspace package list; move the string "inline" so it appears after "fx" and
before "jsonl-store" to restore consistent alphabetical ordering across the
package entries (look for the array containing "fx", "inline", and "jsonl-store"
and reposition "inline" accordingly).
In `@tsconfig.json`:
- Line 29: The tsconfig "references" entry { "path": "inline" } is out of
alphabetical order; move this reference so it appears between the existing "fx"
and "jsonl-store" entries to maintain consistent alphabetical ordering in the
references array (locate the references array in tsconfig.json and reorder the {
"path": "inline" } entry accordingly).
0b023f8 to
66a01e2
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@inline/swc.test.ts`:
- Around line 93-107: swcTransform lacks an explicit return type; update the
function signature for swcTransform to declare the exact return type returned by
transform (referencing transform), e.g. import and use the appropriate type from
the SWC package (like TransformOutput/TransformResult) or use ReturnType<typeof
transform>, and add that type annotation to the swcTransform declaration so the
function signature is explicit.
- Around line 87-107: CI needs to build the WASM plugin before running tests so
the file swc/target/wasm32-wasip1/release/swc_plugin_inline.wasm exists; add a
build step that runs cargo build --manifest-path inline/swc/Cargo.toml --target
wasm32-wasip1 --release (or equivalent) in the workflow before tests run (the
tests call transform via swcTransform which expects the swc_plugin_inline.wasm
generated by the prepack step).
In `@inline/swc/src/lib.rs`:
- Around line 70-78: The has_noinline_annotation function currently scans all
leading comments for the string "@noinline"; update it to only consider block
comments to match JSDoc-style annotations: when iterating leading (from
comments.get_leading(span.lo)) filter/comments.iter() to only check comments
whose kind is CommentKind::Block before testing c.text.contains("@noinline").
Ensure you reference the same symbols (has_noinline_annotation,
comments.get_leading, CommentKind::Block) and return the same boolean result
otherwise.
In `@inline/transform.ts`:
- Around line 174-178: Update the file-extension check so TypeScript files with
.mts and .cts are recognized: modify the isTs determination (the
filename/variable named isTs used before calling parseSync) to also return true
for filenames ending with ".mts" or ".cts" while preserving the existing ".ts"
and ".tsx" handling (and ensure .tsx still maps to TSX/JSX behavior if you have
a separate isTsx flag). Then call parseSync with the same "typescript" syntax
when isTs is true so .mts/.cts are parsed correctly.
---
Duplicate comments:
In `@inline/inline.test.ts`:
- Around line 98-102: The constant helper implements a manual iterator; replace
it with the generator-style iterator to match project guidelines by changing the
constant<T>(value: T): Operation<T> implementation to use a generator method for
[Symbol.iterator] (i.e., define *[Symbol.iterator]() that yields the value) so
the returned Operation<T> uses the `*[Symbol.iterator]` pattern; update any
related type expectations for Operation if needed to accept generator-based
iterators and ensure tests still treat the operation as a single-value iterable.
In `@inline/mod.ts`:
- Around line 4-24: Add a JSDoc comment for the exported function
inline<T>(operation: Operation<T>) that documents the intentional type-safety
trade-off: explain that the implementation uses an unsafe cast (Ok() as
Result<T>) to satisfy the signature and that this makes the actual returned
value effectively unknown at runtime, note any invariants callers must uphold
and risk of misuse, and reference the specific cast/return site (the Ok() as
Result<T> in inline's enter method) so future maintainers understand why the
cast exists and when it can be removed or tightened.
- Around line 62-92: InlineIterator.return currently ignores any value returned
by delegated iterator.return() calls; update the logic in InlineIterator.return
(the branches that call this.current.return(...) and top.return(...)) so that
when iterator.return(...) yields a result with result.done === true you assign
value = result.value and continue unwinding (instead of always returning the
original input value); keep the existing behavior of immediately returning when
result.done === false. Ensure both try/catch sites use this propagation so the
final "return { done: true, value }" returns the propagated value.
In `@inline/package.json`:
- Around line 47-52: The package.json is missing the required "files" array and
the test dependency "expect"; update package.json to add a "files" field that
includes "dist", "mod.ts", and the source directory/files (e.g., your source
folder or globs used in the repo) and add "expect" to devDependencies (so
inline/inline.test.ts can import it); locate the package.json entry point in the
diff (the existing "sideEffects" and "devDependencies" block) and modify it to
include the new "files" key and the "expect" devDependency.
In `@inline/README.md`:
- Around line 8-14: Replace the wordy phrase "The reason is because" in the
explanatory sentence about yield* with a tighter phrasing (e.g., "This is
because" or reword the clause) so the sentence reads more concise and clear;
locate the sentence that starts with "The reason is because when javascript
composes generators via `yield*`" in the README and update it to use the simpler
phrasing while preserving the explanation about intermediate generator frames
and O(depth) resumption cost.
In `@pnpm-workspace.yaml`:
- Line 17: Move the "inline" workspace entry so that the packages list stays
alphabetically ordered; place "inline" after "fx" and before "jsonl-store" in
the pnpm-workspace.yaml packages array to match the existing ordering
convention.
In `@tsconfig.json`:
- Line 29: The tsconfig.json references array is out of alphabetical order
because the { "path": "inline" } entry is misplaced; move the "inline" reference
so it appears after the "fx" entry and before the "jsonl-store" entry (i.e.,
reorder the references array to maintain strict alphabetical sorting by path).
579ffb2 to
255c927
Compare
255c927 to
90d160e
Compare
When Effection operations compose via `yield*`, each level of nesting creates an intermediate JavaScript generator frame. The JS engine must unwind through every one of these frames on each call to `.next()`, `.return()`, or `.throw()`. For deeply nested or recursive operations, this means the cost of resuming a single yield point is O(depth), which is the primary driver of Effection being ~34x slower than vanilla addEventListener in benchmarks. This package provides both a runtime `inline()` function for manual optimization and build-time transforms (esbuild plugin + SWC WASM plugin) that apply the optimization automatically while preserving `yield*` syntax and type safety at the source level. The `inline()` function is a zero-overhead alternative to `yield*` that replaces nested generator delegation with an explicit iterator stack. Instead of: ```ts let value = yield* someOperation(); ``` You write: ```ts let value = yield inline(someOperation()); ``` When the first `inline()` effect enters, it replaces the coroutine's iterator with an `InlineIterator` that manages a stack of iterators in a local reduction loop. Subsequent nested `inline()` calls are caught within that same loop rather than creating additional generator frames. This means a chain of 100 nested inline() calls collapses to a single iterator stepping through a flat loop so that the reducer only ever sees one frame regardless of depth. Because this uses plain `yield` rather than `yield*`, there are no intermediate generator frames at all. The trade-off is that the return type is `unknown` (requiring a cast), and you lose the natural generator stack trace. This is acceptable for a manual optimization that users opt into on hot paths. The `InlineIterator` handles the full generator protocol: - `next()`: steps the current iterator, collapsing inline effects and popping the stack when sub-operations complete - `return()`: unwinds the entire stack, giving each iterator a chance to run its finally blocks (critical for structured concurrency cleanup) - `throw()`: propagates errors up the stack, giving each level a chance to catch (via `raise()` -> `propagate()`) To avoid the manual trade-offs, two build-time transforms are included that automatically rewrite `yield*` into `yield inline(...)` calls. Both produce identical output: they add an import for `$$inline` and convert every `yield* expr()` inside a generator function into `(yield $$inline(expr()))`. The esbuild plugin (`@effectionx/inline/esbuild`) hooks into `onLoad` for `.ts`, `.tsx`, `.js`, and `.jsx` files, running the SWC-based JS transform on each file's source. The SWC WASM plugin (`@effectionx/inline/swc`) is a Rust implementation compiled to `wasm32-wasip1` for use with `@swc/core` or any SWC-based toolchain. Both transforms support opting out: a `"no inline";` directive as the first statement skips the entire file, and a `/** @noinline */` JSDoc annotation on a generator function preserves its `yield*` expressions (while still transforming any nested generators).
90d160e to
0d04a4f
Compare
Motivation
When Effection operations compose via
yield*, each level of nesting creates an intermediate JavaScript generator frame. The JS engine must unwind through every one of these frames on each call to.next(),.return(), or.throw(). For deeply nested or recursive operations, this means the cost of resuming a single yield point is O(depth), which is the primary driver of Effection being ~34x slower than vanilla addEventListener in benchmarks.Approach
This package provides both a runtime
inline()function for manual optimization and build-time transforms (esbuild plugin + SWC WASM plugin) that apply the optimization automatically while preservingyield*syntax and type safety at the source level.The
inline()function is a zero-overhead alternative toyield*that replaces nested generator delegation with an explicit iterator stack. Instead of:You write:
When the first
inline()effect enters, it replaces the coroutine's iterator with anInlineIteratorthat manages a stack of iterators in a local reduction loop. Subsequent nestedinline()calls are caught within that same loop rather than creating additional generator frames. This means a chain of 100 nested inline() calls collapses to a single iterator stepping through a flat loop so that the reducer only ever sees one frame regardless of depth.Because this uses plain
yieldrather thanyield*, there are no intermediate generator frames at all. The trade-off is that the return type isunknown(requiring a cast), and you lose the natural generator stack trace. This is acceptable for a manual optimization that users opt into on hot paths.The
InlineIteratorhandles the full generator protocol:next(): steps the current iterator, collapsing inline effects andpopping the stack when sub-operations complete
return(): unwinds the entire stack, giving each iterator a chanceto run its finally blocks (critical for structured concurrency
cleanup)
throw(): propagates errors up the stack, giving each level achance to catch (via
raise()->propagate())To avoid the manual trade-offs, two build-time transforms are included that automatically rewrite
yield*intoyield inline(...)calls. Both produce identical output: they add an import for$$inlineand convert everyyield* expr()inside a generator function into(yield $$inline(expr())).The esbuild plugin (
@effectionx/inline/esbuild) hooks intoonLoadfor.ts,.tsx,.js, and.jsxfiles, running the SWC-based JS transform on each file's source. The SWC WASM plugin (@effectionx/inline/swc) is a Rust implementation compiled towasm32-wasip1for use with@swc/coreor any SWC-based toolchain.Both transforms support opting out: a
"no inline";directive as the first statement skips the entire file, and a/** @noinline */JSDoc annotation on a generator function preserves itsyield*expressions (while still transforming any nested generators).