Skip to content

feat: Implements cf accessor on Request#122

Merged
dom96 merged 1 commit into
mainfrom
dominik/request-cf-accessor
Jun 12, 2026
Merged

feat: Implements cf accessor on Request#122
dom96 merged 1 commit into
mainfrom
dominik/request-cf-accessor

Conversation

@dom96

@dom96 dom96 commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

This accessor is missing, so users would instead need to use request.js_object.cf manually to access it, which isn't ideal.

@ask-bonk

ask-bonk Bot commented Jun 11, 2026

Copy link
Copy Markdown

The code looks clean. Let me review the overall PR:

  1. Implementation (_workers.py): The cf property is clean and follows the existing pattern (same as body using _jsnull_to_none). The type annotation JsProxy | None is correct. The docstring is clear and includes a link to the relevant documentation.

  2. Tests (test_sdk.py): The test covers both cases — a newly constructed Request without cf (returns None) and an incoming request through workerd where cf is present. The JsProxy import was already present in the file.

  3. Placement: The property is placed between body_used and cache, which is reasonable alphabetical ordering.

There are no logic bugs, security issues, backward compatibility concerns, or other actionable problems. The change is a straightforward addition of a missing accessor property that follows existing patterns in the codebase.

LGTM

github run

@dom96
dom96 force-pushed the dominik/request-cf-accessor branch from d45b0fc to 1434309 Compare June 11, 2026 17:04
@dom96
dom96 merged commit 5777f80 into main Jun 12, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants