-
-
Notifications
You must be signed in to change notification settings - Fork 448
feat: idempotent cookies #1641
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: idempotent cookies #1641
Conversation
WalkthroughRemoved FNV-1a hash-based change-detection and related caching from the Cookie class; cookie values are assigned directly (no decoding), jar entries are always ensured, and createCookieJar always constructs merged Cookies. Tests updated to expect Set-Cookie on modifications. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/cookies.tstest/cookie/unchanged.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/cookies.ts (1)
src/index.ts (2)
Cookie(8176-8176)store(248-250)
🔇 Additional comments (5)
src/cookies.ts (4)
128-130: LGTM: Immutable initial configuration.Making the
initialpropertyreadonlyprevents accidental mutation after construction, which is a good defensive practice.
157-160: LGTM: Simplified value assignment.The value setter now directly assigns without hash-based equality checks, making the behavior deterministic and idempotent. This aligns with the PR objectives.
293-296: LGTM: Consistent Cookie instance creation.The Proxy now always constructs a new
Cookieinstance with merged configurations, ensuring deterministic behavior. This supports the idempotent goal.
323-323: No action needed. Thecookielibrary'sparse()function automatically decodes URI components by default, and line 323 correctly uses the decoded value without attempting additional decoding. This is the proper implementation.test/cookie/unchanged.test.ts (1)
111-111: LGTM: Clearer test name.The renamed test name "cookie handling should be idempotent" better reflects the PR's objectives.
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/cookies.tstest/cookie/unchanged.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/cookies.ts (1)
src/index.ts (2)
Cookie(8176-8176)store(248-250)
🔇 Additional comments (5)
src/cookies.ts (3)
157-160: LGTM! Simplified value setter aligns with idempotent behavior.The removal of hash-based optimization ensures consistent, predictable behavior. Every value assignment now updates the jar, which is intentional for idempotent cookie handling.
293-296: LGTM! Consistent cookie construction supports idempotent behavior.The Proxy now always constructs a new
Cookieinstance with mergedinitialandstorevalues, ensuring consistent state for each access. This aligns with the PR's goal of idempotent, predictable cookie handling.
323-323: Thecookielibrary'sparse()function (v1.1.1) automatically decodes cookie values usingdecodeURIComponentby default, so the removal of the redundantdecode()call is safe and correct.test/cookie/unchanged.test.ts (2)
111-111: LGTM! Test name accurately reflects idempotent behavior.The new name "should idempotently create headers on cookie modifications" correctly describes the test's purpose: modifications produce consistent header output regardless of whether the value changed.
135-135: LGTM! Assertion aligns with idempotent cookie behavior.The test correctly expects 1 Set-Cookie header when modifying a cookie, even if the value is the same as the incoming cookie. This validates the idempotent behavior where modifications consistently produce headers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/cookies.ts (1)
137-141: Line 138 assigns the reference directly and is then immediately overwritten.Two issues:
Line 138 assigns
this.initialdirectly tothis.jar[this.name], creating a reference rather than a shallow copy. While line 140 immediately overwrites this assignment, making it effectively dead code, the pattern is inconsistent with line 158 which correctly creates a shallow copy.Line 138 appears redundant since line 140 unconditionally assigns
jartothis.jar[this.name], which would create the entry regardless.🔎 Proposed fix to remove redundant line
set cookie(jar: ElysiaCookie) { - if (!(this.name in this.jar)) this.jar[this.name] = this.initial this.jar[this.name] = jar }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/cookies.tstest/cookie/unchanged.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/cookies.ts (1)
src/index.ts (2)
Cookie(8176-8176)store(248-250)
🔇 Additional comments (4)
test/cookie/unchanged.test.ts (1)
111-136: LGTM! Test correctly validates idempotent cookie behavior.The test rename and updated assertion correctly reflect the new idempotent behavior where setting a cookie value triggers a Set-Cookie header regardless of whether the value changed. This aligns with the PR objectives to simplify cookie handling by removing hash-based optimization.
src/cookies.ts (3)
128-128: Helpful clarification comment.The comment effectively explains the purpose of the
jarparameter and its relationship to response headers.
293-296: Correct implementation for idempotent cookie creation.Always constructing a new Cookie instance with merged initial values ensures consistent, deterministic behavior. This aligns with the PR objective to simplify cookie handling by removing conditional logic and hash-based optimizations.
323-323: Simplified cookie value handling by removing decode step.The removal of URI decoding is consistent with the PR objectives to simplify cookie handling and remove unnecessary processing. The cookie library's
parsefunction already handles the necessary decoding when parsing the cookie string.
I explained most of the motivation for PR here.
Main points:
This is an example how cookie caching can be implemented via plugin in case someone find this behavior beneficial.
Making decoupled and pluggable architecture ensures that Elysia remains simple and customizable.
Summary by CodeRabbit
Bug Fixes
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.