Skip to content

Commit a690aa1

Browse files
authored
Merge pull request #11 from Jobflow-io/refactor/unwrap-safe-methods
remove effect wrap for safe methods
2 parents a50d5f2 + 38661e1 commit a690aa1

File tree

14 files changed

+409
-116
lines changed

14 files changed

+409
-116
lines changed
Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,141 @@
1+
---
2+
name: implement-playwright-method
3+
description: Implement a Playwright method in effect-playwright by analyzing source code and mapping types
4+
---
5+
6+
## What I do
7+
8+
I guide you through implementing a Playwright method in the `effect-playwright` library. This involves analyzing the original Playwright source code to determine behavior (throwing vs. non-throwing) and mapping types to the Effect ecosystem (Effect, Option, Stream).
9+
10+
## When to use me
11+
12+
Use this skill when you need to add a missing method to `effect-playwright` or update an existing one.
13+
14+
## Procedure
15+
16+
### 1. Locate Playwright Source Code
17+
18+
First, find the implementation of the method in the Playwright codebase to understand its behavior.
19+
20+
- Look in `context/playwright/packages/playwright-core/src/client/`.
21+
- Common files: `page.ts`, `locator.ts`, `browser.ts`, `frame.ts`, `elementHandle.ts`.
22+
23+
### 2. Analyze the Method
24+
25+
Determine if the method can throw and what it returns. **Do not blindly follow existing patterns in `effect-playwright` if they wrap safe synchronous methods in Effects.**
26+
27+
#### Can it throw?
28+
29+
- **Async / Promise-based**: If the method returns a `Promise`, it interacts with the Playwright server and **can throw** (e.g., timeouts, target closed).
30+
- **Mapping**: Map to `Effect<Return, PlaywrightError>`.
31+
- **Sync but Unsafe**: If a synchronous method performs validation or logic that explicitly throws errors.
32+
- **Mapping**: Map to `Effect<Return, PlaywrightError>` (using `Effect.sync` or `Effect.try`).
33+
- **Sync and Safe**: If the method purely returns a property or creates a new object without side effects or throwing (e.g., `url()`, `locator()`, `getByRole()`).
34+
- **Mapping**: **Map 1:1**. Return the value directly. Do NOT wrap in `Effect`.
35+
- **Preference**: Prefer 1:1 mappings over abstractions like getters. If Playwright uses a method `page.url()`, use `page.url()` in the effect wrapper, not a getter `page.url`.
36+
37+
#### Return Type Mapping
38+
39+
- **`Promise<T>`** -> `Effect<T, PlaywrightError>`
40+
- **`Promise<void>`** -> `Effect<void, PlaywrightError>`
41+
- **`T` (Safe Sync)** -> `T` (Direct return)
42+
- **`T` (Factory)** -> `Wrapper<T>` (e.g., `PlaywrightLocator.Service`)
43+
- **`T | null`** -> `Option<T>` (if sync) or `Effect<Option<T>, PlaywrightError>` (if async)
44+
- **Playwright Object (e.g., `Page`)** -> **Wrapped Object (e.g., `PlaywrightPage`)**
45+
46+
### 3. Define the Interface
47+
48+
Add the method to the Service interface in the corresponding `src/X.ts` file (e.g., `PlaywrightPageService` in `src/page.ts`).
49+
50+
**Example (Async Method - Throws):**
51+
52+
```typescript
53+
/**
54+
* Click the element.
55+
* @see {@link Page.click}
56+
*/
57+
readonly click: (
58+
selector: string,
59+
options?: Parameters<Page["click"]>[1]
60+
) => Effect.Effect<void, PlaywrightError>;
61+
```
62+
63+
**Example (Sync Factory - Safe):**
64+
65+
```typescript
66+
/**
67+
* Creates a locator.
68+
* @see {@link Page.locator}
69+
*/
70+
readonly locator: (
71+
selector: string,
72+
options?: Parameters<Page["locator"]>[1]
73+
) => typeof PlaywrightLocator.Service;
74+
```
75+
76+
**Example (Sync Method - Safe):**
77+
78+
```typescript
79+
/**
80+
* Returns the page URL.
81+
* @see {@link Page.url}
82+
*/
83+
readonly url: () => string;
84+
```
85+
86+
**Example (Nullable Return):**
87+
88+
```typescript
89+
/**
90+
* Returns the text content (Async).
91+
* @see {@link Locator.textContent}
92+
*/
93+
readonly textContent: Effect.Effect<Option.Option<string>, PlaywrightError>;
94+
```
95+
96+
### 4. Implement the Method
97+
98+
Implement the method in the `make` function of the implementation class (e.g., `PlaywrightPage.make`).
99+
100+
- **Async Methods**: Use `useHelper(originalObject)`.
101+
102+
```typescript
103+
click: (selector, options) => use((p) => p.click(selector, options)),
104+
```
105+
106+
- **Sync Safe Methods**: Return directly.
107+
108+
```typescript
109+
url: () => page.url(),
110+
```
111+
112+
- **Factories**: Return the wrapped object directly.
113+
114+
```typescript
115+
locator: (selector, options) => PlaywrightLocator.make(page.locator(selector, options)),
116+
```
117+
118+
- **Nullable Returns**: Use `Option.fromNullable`.
119+
120+
```typescript
121+
// Async nullable
122+
textContent: use((l) => l.textContent()).pipe(
123+
Effect.map(Option.fromNullable)
124+
),
125+
126+
// Sync nullable safe
127+
attribute: (name) => Option.fromNullable(element.getAttribute(name)),
128+
```
129+
130+
- **Returning Playwright Objects**: Wrap them.
131+
```typescript
132+
// Async returning object (e.g., waitForEvent returning a Page)
133+
waitForPopup: use((p) => p.waitForEvent("popup")).pipe(
134+
Effect.map(PlaywrightPage.make)
135+
),
136+
```
137+
138+
### 5. Verify
139+
140+
- Ensure types match `PlaywrightXService`.
141+
- Run `npm run typecheck` (or equivalent) to verify implementation.
Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
1+
---
2+
name: verify-playwright-method-throws
3+
description: Analyze a Playwright method's source code to determine if it can throw errors (sync or async).
4+
---
5+
6+
## What I do
7+
I perform a deep analysis of a Playwright method's implementation to determine if it can throw an error. This involves tracing execution paths, checking for asynchronous operations (IPC calls), and identifying explicit `throw` statements or validations.
8+
9+
## When to use me
10+
Use this skill when you need to decide whether a Playwright method should be wrapped in an `Effect` (if it can throw) or mapped 1:1 (if it is guaranteed safe).
11+
12+
## Procedure
13+
14+
### 1. Locate the Implementation
15+
Find the method definition in the Playwright codebase.
16+
- **Client-side Logic**: `context/playwright/packages/playwright-core/src/client/` (e.g., `page.ts`, `locator.ts`).
17+
- **Protocol Definition**: `context/playwright/packages/protocol/src/protocol.yml` (can help identify commands).
18+
19+
### 2. Analyze Direct Implementation
20+
Examine the method body.
21+
22+
- **Async / Promise-returning**:
23+
- Does it return a `Promise`?
24+
- Does it use `await`?
25+
- **Verdict**: Almost always **CAN THROW** (network errors, timeouts, target closed).
26+
27+
- **Synchronous**:
28+
- Does it contain `throw new Error(...)`?
29+
- Does it call other methods that might throw synchronously?
30+
- Does it validate arguments? (e.g., `assert(...)`, checks on `options`).
31+
32+
### 3. Trace Deep Calls (The "Deep Check")
33+
If the method calls other internal methods, trace them.
34+
35+
- **IPC Calls**: Does it call `this._channel.send(...)` or similar?
36+
- **Verdict**: **CAN THROW** (connection issues, server-side errors).
37+
- **Frame/Context Delegation**: Does it delegate to `this._mainFrame` or `this._context`?
38+
- **Action**: Check the implementation of the delegated method in `frame.ts` or `browserContext.ts`.
39+
- **Utility Functions**: Does it call helpers like `assertMaxArguments`, `parseResult`, `serializeArgument`?
40+
- **Verdict**: These can throw validation errors.
41+
42+
### 4. Classify the Method
43+
44+
Based on the analysis, classify the method into one of two categories:
45+
46+
#### A. **Guaranteed Safe (Cannot Throw)**
47+
- **Criteria**:
48+
- Synchronous.
49+
- No `throw` statements.
50+
- No calls to methods that throw.
51+
- No IPC / Protocol usage.
52+
- Pure data access (e.g., returning a private field, creating a wrapper).
53+
- **Examples**:
54+
- `page.url()` (returns a string from local state).
55+
- `page.locator(...)` (creates a locator object).
56+
- `page.viewportSize()` (returns cached size).
57+
- **Result**: **Map 1:1**. Do not wrap in Effect.
58+
59+
#### B. **Can Throw (Unsafe)**
60+
- **Criteria**:
61+
- Returns a `Promise` (Async).
62+
- Performs IPC / Network calls.
63+
- Contains explicit validation that throws `Error`.
64+
- Accesses resources that might be closed/destroyed.
65+
- **Examples**:
66+
- `page.click(...)` (Async, IPC).
67+
- `page.title()` (Async, IPC - even if it seems like a property).
68+
- `page.evaluate(...)` (Async, script execution).
69+
- `locator.elementHandle()` (Async).
70+
- **Result**: **Wrap in `Effect<T, PlaywrightError>`**.
71+
72+
## Output Format
73+
When reporting the result of this skill, please provide:
74+
1. **Method Name**: The method analyzed.
75+
2. **File Location**: Where the implementation was found.
76+
3. **Analysis**: Brief explanation of the code path (e.g., "Calls `this._channel.click`, which is an IPC call").
77+
4. **Conclusion**: "Can Throw" or "Cannot Throw".

README.md

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -37,8 +37,7 @@ const program = Effect.gen(function* () {
3737
const page = yield* browser.newPage();
3838

3939
yield* page.goto("https://example.com");
40-
const title = yield* page.title;
41-
console.log(`Page title: ${title}`);
40+
console.log(`Page title: ${page.title()}`);
4241
}).pipe(Effect.scoped, Effect.provide(Playwright.layer));
4342

4443
await Effect.runPromise(program);
@@ -99,7 +98,7 @@ import { Effect } from "effect";
9998
import { chromium } from "playwright-core";
10099

101100
const liveLayer = PlaywrightEnvironment.layer(chromium, {
102-
headless: true /** any other launch options */,
101+
headless: false /** any other launch options */,
103102
});
104103

105104
const program = Effect.gen(function* () {
@@ -148,8 +147,7 @@ const program = Effect.gen(function* () {
148147
yield* page.eventStream("request").pipe(
149148
Stream.runForEach((request) =>
150149
Effect.gen(function* () {
151-
const url = yield* request.url;
152-
yield* Effect.log(`Request: ${url}`);
150+
yield* Effect.log(`Request: ${request.url()}`);
153151
}),
154152
),
155153

src/browser.test.ts

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ layer(Playwright.layer)("PlaywrightBrowser", (it) => {
3232
const playwright = yield* Playwright;
3333
const browser = yield* playwright.launchScoped(chromium);
3434

35-
const type = yield* browser.browserType;
35+
const type = browser.browserType();
3636
assert.strictEqual(type.name(), "chromium");
3737
}),
3838
);
@@ -42,7 +42,7 @@ layer(Playwright.layer)("PlaywrightBrowser", (it) => {
4242
const playwright = yield* Playwright;
4343
const browser = yield* playwright.launchScoped(chromium);
4444

45-
const version = yield* browser.version;
45+
const version = browser.version();
4646
assert.isString(version);
4747
assert.isNotEmpty(version);
4848
}),
@@ -66,11 +66,11 @@ layer(Playwright.layer)("PlaywrightBrowser", (it) => {
6666
const playwright = yield* Playwright;
6767
const browser = yield* playwright.launchScoped(chromium);
6868

69-
const initialContexts = yield* browser.contexts;
69+
const initialContexts = browser.contexts();
7070
assert.strictEqual(initialContexts.length, 0);
7171

7272
yield* browser.newContext();
73-
const contextsAfterOne = yield* browser.contexts;
73+
const contextsAfterOne = browser.contexts();
7474
assert.strictEqual(contextsAfterOne.length, 1);
7575
}),
7676
);
@@ -108,7 +108,7 @@ layer(Playwright.layer)("PlaywrightBrowser", (it) => {
108108
const browser = yield* playwright.launchScoped(chromium);
109109

110110
yield* browser.newPage();
111-
const contexts = yield* browser.contexts;
111+
const contexts = browser.contexts();
112112
assert.strictEqual(contexts.length, 1);
113113

114114
const pages = yield* contexts[0].pages;
@@ -129,18 +129,18 @@ layer(Playwright.layer)("PlaywrightBrowser", (it) => {
129129
yield* Effect.scoped(
130130
Effect.gen(function* () {
131131
yield* browser.newContext();
132-
const contexts = yield* browser.contexts;
132+
const contexts = browser.contexts();
133133
assert.strictEqual(contexts.length, 1);
134134
}),
135135
);
136136

137-
const contextsAfter = yield* browser.contexts;
137+
const contextsAfter = browser.contexts();
138138
assert.strictEqual(contextsAfter.length, 0);
139139
}),
140140
);
141141

142142
assert.isDefined(capturedBrowser);
143-
const isConnected = yield* capturedBrowser?.isConnected;
143+
const isConnected = capturedBrowser?.isConnected();
144144
assert.isFalse(isConnected);
145145
}),
146146
);
@@ -158,7 +158,7 @@ layer(Playwright.layer)("PlaywrightBrowser", (it) => {
158158
assert.strictEqual(Chunk.size(events), 1);
159159

160160
const firstEvent = yield* Chunk.head(events);
161-
assert.strictEqual(yield* firstEvent.version, yield* browser.version);
161+
assert.strictEqual(firstEvent.version(), browser.version());
162162
}),
163163
);
164164
});

src/browser.ts

Lines changed: 8 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -67,9 +67,7 @@ export interface PlaywrightBrowserService {
6767
* An Effect that returns the list of all open browser contexts.
6868
* @see {@link Browser.contexts}
6969
*/
70-
readonly contexts: Effect.Effect<
71-
Array<typeof PlaywrightBrowserContext.Service>
72-
>;
70+
readonly contexts: () => Array<typeof PlaywrightBrowserContext.Service>;
7371

7472
readonly newContext: (
7573
options?: NewContextOptions,
@@ -83,18 +81,18 @@ export interface PlaywrightBrowserService {
8381
* An Effect that returns the browser type (chromium, firefox or webkit) that the browser belongs to.
8482
* @see {@link Browser.browserType}
8583
*/
86-
readonly browserType: Effect.Effect<BrowserType>;
84+
readonly browserType: () => BrowserType;
8785

8886
/**
8987
* An Effect that returns the version of the browser.
9088
* @see {@link Browser.version}
9189
*/
92-
readonly version: Effect.Effect<string>;
90+
readonly version: () => string;
9391
/**
9492
* An Effect that returns whether the browser is connected.
9593
* @see {@link Browser.isConnected}
9694
*/
97-
readonly isConnected: Effect.Effect<boolean>;
95+
readonly isConnected: () => boolean;
9896

9997
/**
10098
* Creates a stream of the given event from the browser.
@@ -129,19 +127,17 @@ export class PlaywrightBrowser extends Context.Tag(
129127
newPage: (options) =>
130128
use((browser) => browser.newPage(options).then(PlaywrightPage.make)),
131129
close: use((browser) => browser.close()),
132-
contexts: Effect.sync(() =>
133-
browser.contexts().map(PlaywrightBrowserContext.make),
134-
),
130+
contexts: () => browser.contexts().map(PlaywrightBrowserContext.make),
135131
newContext: (options) =>
136132
Effect.acquireRelease(
137133
use((browser) =>
138134
browser.newContext(options).then(PlaywrightBrowserContext.make),
139135
),
140136
(context) => context.close.pipe(Effect.ignoreLogged),
141137
),
142-
browserType: Effect.sync(() => browser.browserType()),
143-
version: Effect.sync(() => browser.version()),
144-
isConnected: Effect.sync(() => browser.isConnected()),
138+
browserType: () => browser.browserType(),
139+
version: () => browser.version(),
140+
isConnected: () => browser.isConnected(),
145141
eventStream: <K extends keyof BrowserEvents>(event: K) =>
146142
Stream.asyncPush<BrowserEvents[K]>((emit) =>
147143
Effect.acquireRelease(

0 commit comments

Comments
 (0)