Improved API output serializer to avoid per-request allocations#28953
Improved API output serializer to avoid per-request allocations#28953frenck wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ghost/core/core/server/api/endpoints/utils/serializers/output/all.js`:
- Around line 10-13: The `removeXBY` walker in `all.js` now skips deleting
`published_by` whenever the value is an object or array, because the `else if`
ties deletion to the non-object branch. Update the logic so `published_by` is
removed independently of recursion—either delete it before descending or keep
the `published_by` check separate from the `typeof value === 'object'` guard in
`removeXBY`.
In `@ghost/core/test/unit/api/canary/utils/serializers/output/all.test.js`:
- Around line 88-89: The serializer tests currently only check that
`published_by` is `undefined`, which still allows the property to exist with an
undefined value. Update the assertions in `output/all.test.js` for
`response.posts[0]` and `response.posts[0].tiers[0]` to also verify the key is
absent, mirroring the `in`-operator approach already used in the null test. Use
the existing `response.posts` checks in the test case to locate and adjust both
nested object assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5fe02064-5bff-468e-8975-6450b4cae571
📒 Files selected for processing (2)
ghost/core/core/server/api/endpoints/utils/serializers/output/all.jsghost/core/test/unit/api/canary/utils/serializers/output/all.test.js
no ref - removeXBY runs on every API response and walked the full tree allocating a fresh `['published_by']` array for every key and calling lodash for every value - replaced the per-key includes() with a direct comparison, the lodash type checks with a typeof check, and dropped the now-unused lodash import - behavior is unchanged: published_by is still stripped anywhere in the response, including null values and nested resources, covered by the extended unit test
8f78bfe to
13b546c
Compare
Got some code for us? Awesome 🎊!
Please take a minute to explain the change you're making:
Why are you making it?
removeXBYin the output serializer runsafterevery API response and recursively walks the entire response tree. In the hot path it allocated a fresh['published_by']array for every key of every object and called lodash (_.isObject/_.isArray) for every value. That is pure per-request overhead on a path every Content and Admin API response goes through. I found it while profiling the Content APIposts/endpoint.What does it do?
Replaces the per-key
['published_by'].includes(key)with a directkey === 'published_by'comparison, swaps the lodash type checks for a plaintypeofcheck (which already covers arrays, so the redundant_.isArrayis gone), switchesObject.entriestoObject.keysto drop the tuple allocations, and removes the now-unused lodash import. In a CPU profile of repeatedposts/requests this cut the function's self time by roughly 75%.Why is this something Ghost users or developers need?
It is a no-risk reduction in CPU and allocation (so less GC pressure) on a universal API hot path, with no change in behavior or output.
Behavior is unchanged:
published_byis still stripped from anywhere in the response, includingnullvalues and nested resources. The existing unit test is extended to cover those cases.Please check your PR against these items:
We appreciate your contribution! 🙏