feat(process): add processApi with middleware support#159
feat(process): add processApi with middleware support#159
Conversation
|
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 middleware-capable public API Changes
Sequence DiagramsequenceDiagram
participant Client
participant Exec
participant DoExec
participant API
participant Middleware
participant OS
Client->>Exec: exec(cmd, opts)
Exec->>DoExec: route via doExec
DoExec->>API: operations.exec(cmd, opts)
API->>Middleware: around(exec) invoked
Middleware->>Middleware: inspect/modify args
Middleware->>API: call next(...)
API->>OS: create/run process
OS-->>API: Process
API-->>DoExec: Process
DoExec-->>Exec: Process
Exec-->>Client: Process
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@process/package.json`:
- Around line 14-16: Update the package.json version field to a minor bump to
reflect the new public API surface (e.g., change "version": "0.7.2" to "0.8.0");
ensure the package.json "version" value is updated consistently and include an
entry in the release notes/changelog if present so the new middleware-capable
API is recorded before publishing.
In `@process/README.md`:
- Around line 230-233: The heading "#### Capturing process executions for
testing" is jumping levels; change this heading to one level higher (use "###
Capturing process executions for testing") so the Markdown increments by one
level; update the heading text in the README.md where "Capturing process
executions for testing" appears to fix MD001 and keep surrounding headings
consistent with the rest of the document.
In `@process/test/exec.test.ts`:
- Around line 526-569: The tests use the shell builtin "echo" which fails on
Windows; update the processApi.around tests to invoke a real executable instead
of a shell builtin by calling exec with a binary and args (e.g., use "node" with
"-e" and a small console.log payload) so the created command tokens and
middleware behavior are preserved; modify both uses of exec("echo ...") in the
tests around the createProcess middleware (refer to processApi.around,
createProcess, exec, and spawn) to call exec with the executable and argument
array form to ensure cross-platform compatibility.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@process/package.json`:
- Around line 14-15: The peerDependencies entry currently pins effection to a
pre-release ("^4.1.0-alpha.3") which is overly restrictive; update the
package.json peerDependencies key for "effection" to the project-standard range
"^3 || ^4" so consumers of both Effection 3.x and 4.x stable releases are
supported; modify the "peerDependencies" block (the "effection" entry)
accordingly and run a quick install/test to verify no semver conflicts.
In `@process/README.md`:
- Around line 6-9: Update the version note text that currently reads "Note:
Starting with version 0.2.0, this package requires Effection v4.1 or greater..."
to reflect the actual release that introduced the Effection v4.1 requirement:
locate the markdown paragraph containing that exact sentence (the "Note:
Starting with version 0.2.0..." line) in README and change "0.2.0" to the
correct version number for this release so the note accurately states which
version requires Effection v4.1.
- Around line 236-249: The example's logged output is incorrect because the
middleware in processApi.around's createProcess receives parsed command and args
(args[0] is the command name like "git" or "npm"), so change the example to
either join the command and its arguments before pushing (e.g. push `${args[0]}
${args.slice(1).join(' ')}`) or update the expected console output to reflect
only the command names (e.g. ["git","npm"]); update the example using the
createProcess middleware and exec invocation to show consistent input and
output.
| "peerDependencies": { | ||
| "effection": "^3 || ^4" | ||
| "effection": "^4.1.0-alpha.3" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
python - <<'PY'
import json
import urllib.request
try:
data = json.load(urllib.request.urlopen("https://registry.npmjs.org/effection"))
dist_tags = data.get("dist-tags", {})
versions = list(data.get("versions", {}).keys())
print("=== dist-tags ===")
for tag, version in dist_tags.items():
print(f"{tag}: {version}")
print("\n=== Search for 4.1.0-alpha.3 ===")
print(f"4.1.0-alpha.3 exists: {'4.1.0-alpha.3' in versions}")
print("\n=== Recent alpha/beta versions ===")
alpha_beta = [v for v in versions if 'alpha' in v or 'beta' in v]
alpha_beta.sort()
print(f"Found {len(alpha_beta)} pre-release versions")
if alpha_beta:
print(f"Latest pre-releases: {alpha_beta[-5:]}")
print("\n=== Latest stable versions ===")
stable = [v for v in versions if not ('alpha' in v or 'beta' in v or 'rc' in v)]
stable.sort()
print(f"Latest stable: {stable[-3:]}")
except Exception as e:
print(f"Error: {e}")
PYRepository: thefrontside/effectionx
Length of output: 201
🏁 Script executed:
# Check the actual peerDependencies policy in the codebase
find . -name "package.json" -o -name "*policies*" | head -20
cat .policies/index.md 2>/dev/null || echo "Not found in expected location"Repository: thefrontside/effectionx
Length of output: 2136
🏁 Script executed:
cat .policies/version-bump.md .policies/package-json-metadata.mdRepository: thefrontside/effectionx
Length of output: 5067
🏁 Script executed:
cat process/package.jsonRepository: thefrontside/effectionx
Length of output: 1051
Update peerDependencies to match project guidance: "^3 || ^4"
Current constraint "^4.1.0-alpha.3" is more restrictive than the project standard and uses a pre-release as the minimum version. Consumers need compatibility with both Effection 3.x and 4.x stable releases. Update to "^3 || ^4" unless the pre-release version is intentionally required (e.g., for new APIs).
🤖 Prompt for AI Agents
In `@process/package.json` around lines 14 - 15, The peerDependencies entry
currently pins effection to a pre-release ("^4.1.0-alpha.3") which is overly
restrictive; update the package.json peerDependencies key for "effection" to the
project-standard range "^3 || ^4" so consumers of both Effection 3.x and 4.x
stable releases are supported; modify the "peerDependencies" block (the
"effection" entry) accordingly and run a quick install/test to verify no semver
conflicts.
process/src/exec.ts
Outdated
| * Create a process operation. | ||
| * This is the core operation that middleware can intercept. | ||
| */ | ||
| createProcess(cmd: string, opts: ExecOptions): Operation<Process>; |
There was a problem hiding this comment.
I think the APi should be exec and daemon, not createProcess
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
process/src/exec.ts (1)
136-202: 🧹 Nitpick | 🔵 TrivialConsider potential double-execution edge case.
Each method (
[Symbol.iterator],join,expect) independently callsdoExec(), which creates a new process. While the typical usage patterns (exec().join()orlet p = yield* exec()) are safe, the following would unexpectedly spawn two processes:let e = exec("cmd"); yield* e; // process 1 yield* e.join(); // process 2 (Exec.join, not Process.join)This appears to be the existing design, not a regression. Consider adding a JSDoc note to clarify the intended usage patterns.
🤖 Fix all issues with AI agents
In `@process/mod.ts`:
- Around line 1-3: Remove the redundant explicit re-export of processApi: the
existing export * from "./src/exec.ts" already re-exports processApi, so delete
the line exporting processApi and, if you want to keep discoverability, add a
short doc comment above the barrel export (export * from "./src/exec.ts")
mentioning that processApi is available from this module; reference symbols:
processApi and the export * from "./src/exec.ts".
In `@process/src/exec.ts`:
- Line 6: The import of createApi in process/src/exec.ts is invalid for the
declared Effection version and will break at runtime; remove createApi from the
effection import list and refactor any code in exec.ts that calls createApi to
use a supported Effection primitive (e.g., replace with an equivalent using
available APIs such as createResource/spawn/Operation or a small local wrapper),
or stub the functionality with a clear TODO comment; once
thefrontside/effection#1116 lands and you bump the effection dependency, restore
createApi and remove the temporary workaround, and add/adjust tests to cover the
altered code path to prevent regressions.
| export * from "./src/exec.ts"; | ||
| export { type Daemon, daemon } from "./src/daemon.ts"; | ||
| // Re-export processApi explicitly for better discoverability | ||
| export { processApi } from "./src/exec.ts"; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant re-export of processApi.
Line 1's export * already includes processApi from ./src/exec.ts. The explicit re-export on line 3 is redundant. The discoverability comment is appreciated, but consider using a doc comment on the barrel export instead.
♻️ Suggested simplification
-export * from "./src/exec.ts";
-// Re-export processApi explicitly for better discoverability
-export { processApi } from "./src/exec.ts";
+/** `@see` {`@link` processApi} for middleware support */
+export * from "./src/exec.ts";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export * from "./src/exec.ts"; | |
| export { type Daemon, daemon } from "./src/daemon.ts"; | |
| // Re-export processApi explicitly for better discoverability | |
| export { processApi } from "./src/exec.ts"; | |
| /** `@see` {`@link` processApi} for middleware support */ | |
| export * from "./src/exec.ts"; |
🤖 Prompt for AI Agents
In `@process/mod.ts` around lines 1 - 3, Remove the redundant explicit re-export
of processApi: the existing export * from "./src/exec.ts" already re-exports
processApi, so delete the line exporting processApi and, if you want to keep
discoverability, add a short doc comment above the barrel export (export * from
"./src/exec.ts") mentioning that processApi is available from this module;
reference symbols: processApi and the export * from "./src/exec.ts".
| import { | ||
| type Api, | ||
| type Operation, | ||
| createApi, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
web_search
Effection v4.1 createApi function documentationRepository: thefrontside/effectionx
Length of output: 165
🏁 Script executed:
# Check package.json to see the declared Effection version
find . -name "package.json" -type f | head -20Repository: thefrontside/effectionx
Length of output: 528
🏁 Script executed:
# Examine the exec.ts file to understand the createApi import
cat -n process/src/exec.ts | head -20Repository: thefrontside/effectionx
Length of output: 623
🏁 Script executed:
# Check the process/package.json for Effection version
cat process/package.jsonRepository: thefrontside/effectionx
Length of output: 1089
🌐 Web query:
Effection v4.1 createApi function
💡 Result:
createApi is not an Effection v4.1 API. In the Effection v4.1.0 (currently documented as 4.1.0-alpha.3) API reference, there is no createApi export—only things like createContext, createScope, createSignal, etc. [1]
If you meant Effector (state manager), that’s where createApi exists:
- Signature:
createApi(store, api): objectWithEvents - Purpose: generates events bound to a store using reducer functions you provide [2]
import { createStore, createApi } from "effector";
const $pos = createStore(0);
const api = createApi($pos, {
left: (pos, n: number) => pos - n,
right: (pos, n: number) => pos + n,
});
api.right(10);
api.left(3);If you tell me whether you’re using Effection or Effector (and which package name you installed), I can point you to the exact equivalent in that library.
Sources: [1] [2]
🌐 Web query:
thefrontside/effection PR 1116 createApi
💡 Result:
PR #1116 in thefrontside/effection is titled “feat: export createApi from main module”. It proposes making createApi part of the public/stable API so users can import it from the main module instead of experimental.ts. [1]
What changes in the PR:
- Adds a documented
createApi(name, core)export inlib/api.ts(wrapper overcreateApiInternal) with JSDoc + examples. [2] - Exports
createApifrom the main module entrypoint:lib/mod.tsnow includesexport { createApi } from "./api.ts";. [2] - Updates tests to import
createApifrom../mod.tsrather than../experimental.ts, plus related test adjustments. [2]
Preview install for the PR build:
npm i https://pkg.pr.new/thefrontside/effection@1116
``` [1]
Sources: [1] PR conversation page, [2] PR “Files changed” diff.
---
🌐 Web query:
`thefrontside/effection PR 1116 merged status`
💡 Result:
PR **#1116** in `thefrontside/effection` (**“feat: export createApi from main module”**) is currently **Open (not merged)**, proposing to merge **1 commit** from `feat/export-createapi` into the `v4-1-alpha` branch, and is **awaiting the requested review** (from `cowboyd`). [1]
---
</details>
**`createApi` is not available in the declared Effection version and will cause runtime errors.**
The code imports `createApi` from `effection`, but this function does not exist in the documented Effection v4.1.0-alpha.3 API (declared in `process/package.json`). Upstream PR thefrontside/effection#1116 ("feat: export createApi from main module") is still open and awaiting review. This package cannot be published until that PR is merged and the Effection version dependency is updated accordingly.
<details>
<summary>🤖 Prompt for AI Agents</summary>
In @process/src/exec.ts at line 6, The import of createApi in
process/src/exec.ts is invalid for the declared Effection version and will break
at runtime; remove createApi from the effection import list and refactor any
code in exec.ts that calls createApi to use a supported Effection primitive
(e.g., replace with an equivalent using available APIs such as
createResource/spawn/Operation or a small local wrapper), or stub the
functionality with a clear TODO comment; once thefrontside/effection#1116 lands
and you bump the effection dependency, restore createApi and remove the
temporary workaround, and add/adjust tests to cover the altered code path to
prevent regressions.
</details>
<!-- fingerprinting:phantom:medusa:ocelot -->
<!-- This is an auto-generated comment by CodeRabbit -->
Add createApi-based processApi that exposes a createProcess operation for middleware interception. This enables logging, mocking, and instrumentation of process execution. - Export processApi from mod.ts - Add middleware tests in exec.test.ts - Update README with middleware documentation and v4.1 disclaimer - Update peer dependency to effection ^4.1.0-alpha.3
…rm tests - Bump version 0.7.2 → 0.8.0 for new public API - Fix README heading level (#### → ###) - Fix trailing code block in README - Use node -e instead of echo for Windows compatibility in tests
- Fix version note from 0.2.0 → 0.8.0 - Fix example output to show actual parsed command (git, npm)
Changed processApi from exposing a single 'createProcess' operation to exposing 'exec' and 'daemon' operations directly. This makes the middleware API more intuitive since users call exec() and daemon(), so those should be the interception points. - Merged daemon.ts into exec.ts - Updated tests to use *exec middleware - Updated README with new API examples Session-ID:
Per CodeRabbit feedback, package.json must include a files field containing dist, mod.ts, and source files. Session-ID:
Add pnpm override to use effection from PR #1116 which provides createApi required for processApi middleware support. Also skip tests that fail due to timing differences in preview effection: - effect-ts: scope teardown timing tests - stream-helpers: valve test
taras
force-pushed
the
feat/process-api
branch
from
6b9bdcb to
b84bb58
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@process/README.md`:
- Line 231: Change the heading "Intercepting daemon calls" (and the similar
heading at line 251) to use level ### instead of #### so the heading level does
not jump after the `processApi` section; locate the occurrences of the text
"Intercepting daemon calls" in README.md and replace the leading four hashes
with three to restore proper heading hierarchy under `processApi`.
In `@process/src/exec.ts`:
- Around line 3-9: Change the import for the type-only symbol Api to a
TypeScript type-only import to improve tree-shaking and clarity: update the
import statement that currently brings in Api, Operation, createApi, resource,
spawn so that Api is imported with the type modifier (keep Operation as-is if
it’s used at runtime). Locate the import at the top of the file (references:
Api, Operation, createApi, resource, spawn) and modify only the Api import to be
type-only.
- Around line 137-146: doExec() is being invoked multiple times (in the
iterator, join(), and expect()), which spawns duplicate processes; change the
implementation to lazily create and cache the single process/operation result
returned by processApi.operations.exec(command, options) (e.g., store it in a
local variable like cachedOp or createdProcess) and have *[Symbol.iterator](),
*join(), and *expect() all yield from that cached value so the process is
created once and reused; if maintaining mutable cached state across yields is
concerning, use Effection's resource/useScope pattern to manage the single
process lifecycle instead.
---
Duplicate comments:
In `@process/mod.ts`:
- Around line 2-3: The explicit re-export of processApi is redundant because the
existing barrel export (export *) already exposes processApi; remove the
explicit line "export { processApi } from \"./src/exec.ts\";" or instead convert
the preceding comment into a JSDoc on the barrel export to preserve
discoverability, leaving only the single barrel export that exposes processApi
(symbol: processApi) and avoiding duplicate exports.
In `@process/package.json`:
- Around line 15-16: The peerDependencies entry currently pins effection to the
pre-release "^4.1.0-alpha.3" for the createApi feature; update the project docs
to clearly state this intentional constraint (add a note in the package README
or CHANGELOG referencing the peerDependencies key and the createApi feature and
why effection 4.1-alpha is required), and add a TODO comment in package.json
maintenance notes to change the version to "^4.1" (or broaden back to "^3 || ^4"
if compatible) once Effection 4.1.0 is stable; keep the existing "effection":
"^4.1.0-alpha.3" entry until that follow-up is done.
In `@process/src/exec.ts`:
- Line 6: The import of createApi in process/src/exec.ts depends on an unmerged
Effection PR (`#1116`); remove the direct import of createApi (and any usage of it
in this module) and replace it with a safe fallback that either (a) uses an
alternative local implementation or helper, or (b) throws a clear runtime error
explaining the upstream dependency until `#1116` is merged; update any functions
that call createApi to use the fallback or guard them behind a feature flag so
the package can be published safely without the upstream PR.
| }); | ||
| ``` | ||
|
|
||
| #### Intercepting daemon calls |
There was a problem hiding this comment.
Fix heading level increment (MD001).
The static analysis flags a heading level jump. The heading #### Intercepting daemon calls follows a code block under ### processApi, so it should be ### to maintain proper hierarchy, or the structure should be reorganized.
✍️ Suggested fix
-#### Intercepting daemon calls
+### Intercepting daemon callsAnd similarly for line 251:
-#### Capturing process executions for testing
+### Capturing process executions for testing📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #### Intercepting daemon calls | |
| ### Intercepting daemon calls |
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 231-231: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@process/README.md` at line 231, Change the heading "Intercepting daemon
calls" (and the similar heading at line 251) to use level ### instead of #### so
the heading level does not jump after the `processApi` section; locate the
occurrences of the text "Intercepting daemon calls" in README.md and replace the
leading four hashes with three to restore proper heading hierarchy under
`processApi`.
| import { | ||
| type Api, | ||
| type Operation, | ||
| createApi, | ||
| resource, | ||
| spawn, | ||
| } from "effection"; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer type imports for type-only symbols.
Api is used only as a type annotation (line 98). Consider using a type-only import for better tree-shaking and clearer intent.
♻️ Suggested change
-import {
- type Api,
- type Operation,
- createApi,
- resource,
- spawn,
-} from "effection";
+import type { Api, Operation } from "effection";
+import { createApi, resource, spawn } from "effection";As per coding guidelines: "Prefer type imports for type-only imports in TypeScript".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@process/src/exec.ts` around lines 3 - 9, Change the import for the type-only
symbol Api to a TypeScript type-only import to improve tree-shaking and clarity:
update the import statement that currently brings in Api, Operation, createApi,
resource, spawn so that Api is imported with the type modifier (keep Operation
as-is if it’s used at runtime). Locate the import at the top of the file
(references: Api, Operation, createApi, resource, spawn) and modify only the Api
import to be type-only.
| function* doExec(): Operation<Process> { | ||
| return yield* processApi.operations.exec(command, options); | ||
| } | ||
|
|
||
| return { | ||
| *[Symbol.iterator]() { | ||
| return yield* createProcess(cmd, opts); | ||
| return yield* doExec(); | ||
| }, | ||
| *join() { | ||
| const process = yield* createProcess(cmd, opts); | ||
| const process = yield* doExec(); |
There was a problem hiding this comment.
Critical: doExec() called multiple times creates multiple processes.
The exec() function's join() and expect() methods each call doExec(), which invokes processApi.operations.exec(). Since the iterator (*[Symbol.iterator]()) also calls doExec(), if a user calls yield* exec(...).join(), both the iterator and join() execute doExec(), spawning two separate processes.
The previous implementation likely stored the process from a single creation. The refactored code needs to ensure the process is created once and reused.
🐛 Proposed fix - create process once and reuse
export function exec(command: string, options: ExecOptions = {}): Exec {
- function* doExec(): Operation<Process> {
- return yield* processApi.operations.exec(command, options);
- }
+ let process: Process | undefined;
+
+ function* getProcess(): Operation<Process> {
+ if (!process) {
+ process = yield* processApi.operations.exec(command, options);
+ }
+ return process;
+ }
return {
*[Symbol.iterator]() {
- return yield* doExec();
+ return yield* getProcess();
},
*join() {
- const process = yield* doExec();
+ const proc = yield* getProcess();
// ... rest uses proc
},
*expect() {
- const process = yield* doExec();
+ const proc = yield* getProcess();
// ... rest uses proc
},
};
}Note: The above fix has a limitation - mutable state across generator yields can be problematic. Consider using Effection's resource or useScope pattern to properly manage the process lifecycle.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@process/src/exec.ts` around lines 137 - 146, doExec() is being invoked
multiple times (in the iterator, join(), and expect()), which spawns duplicate
processes; change the implementation to lazily create and cache the single
process/operation result returned by processApi.operations.exec(command,
options) (e.g., store it in a local variable like cachedOp or createdProcess)
and have *[Symbol.iterator](), *join(), and *expect() all yield from that cached
value so the process is created once and reused; if maintaining mutable cached
state across yields is concerning, use Effection's resource/useScope pattern to
manage the single process lifecycle instead.
1 participant
Motivation
Add middleware support to
@effectionx/processusing Effection's newcreateApifeature, enabling logging, mocking, and instrumentation of process execution for testing and observability.This is part of a series of PRs converting effectionx packages to use the new
createApipattern:@effectionx/fetch- feat(fetch): add fetchApi with middleware support #158@effectionx/process- this PR@effectionx/fs- coming nextApproach
processApiwith acreateProcessoperation that middleware can interceptexec()to use the API internally while maintaining backward compatibilitypkg.pr.new/thefrontside/effection@1116preview forcreateApisupportNote: This requires the Effection PR to be merged first: thefrontside/effection#1116
Summary by CodeRabbit
New Features
Breaking / Public API
Documentation
Tests
Chores