Required the custom fields services on their members consumers - #29485
Merged
Conversation
Contributor
|
Caution Review failedAn error occurred during the review process. Please try again later. Walkthrough
Possibly related PRs
Suggested reviewers: 🚥 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 |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run ghost:test:ci:integration |
✅ Succeeded | 3m 6s | View ↗ |
nx run ghost:test:integration |
✅ Succeeded | 3m 4s | View ↗ |
nx run ghost:test:legacy |
✅ Succeeded | 3m 1s | View ↗ |
nx run ghost:test:e2e |
✅ Succeeded | 2m 32s | View ↗ |
nx run ghost-monorepo:lint:boundaries |
✅ Succeeded | 16s | View ↗ |
nx run-many -t test:unit -p ghost |
✅ Succeeded | 32s | View ↗ |
nx run-many -t lint -p ghost,ghost-monorepo |
✅ Succeeded | 17s | View ↗ |
nx run @tryghost/admin:build |
✅ Succeeded | 10s | View ↗ |
nx run-many --target=build --projects=tag:publi... |
✅ Succeeded | <1s | View ↗ |
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗
☁️ Nx Cloud last updated this comment at 2026-07-21 11:35:04 UTC
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #29485 +/- ##
==========================================
- Coverage 74.60% 74.55% -0.05%
==========================================
Files 1604 1604
Lines 140867 140910 +43
Branches 17179 17194 +15
==========================================
- Hits 105096 105060 -36
- Misses 34716 34819 +103
+ Partials 1055 1031 -24
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Base automatically changed from
chore/tidy-member-custom-fields-comments
to
main
July 21, 2026 10:03
rob-ghost
force-pushed
the
chore/require-custom-fields-service-dependency
branch
2 times, most recently
from
July 21, 2026 10:28
5a584a3 to
d814cae
Compare
MemberBREADService's add() guarded its call into the custom fields service on the request body carrying a custom_fields key. The custom fields service is a required dependency of the members service, and boot constructs it before the members service, so an instance without one cannot exist in a running Ghost and the guard was protecting a state that cannot occur. The dependency is now also inverted: the members service previously received the custom fields module namespace and reached through it as customFieldsService.values on every call, which meant depending on a module-level binding that is undefined until init() runs, so it takes the values service itself as customFieldValues, resolved once in services/members/api.js at construction. Custom fields is initialised in initCore and the members API is built in initServices, so construction is where boot order is expressed rather than each call site late-binding through a namespace. The hand-written typedef restating the service's shape is replaced by a reference to the real exported CustomFieldValuesService type, matching how memberRepository and offersAPI are already typed in that file, because a restated structural typedef can drift from the service it describes and an imported one cannot. The unit fixtures previously built a service that could not exist in production, and now build it the way boot does, sharing a single stub so the factories in that file cannot drift apart. Nothing enforces the dependency at runtime, because a dependency that cannot be absent does not need a guard; making it non-optional in the type system belongs with converting the file to TypeScript. No behaviour change for any request.
rob-ghost
force-pushed
the
chore/require-custom-fields-service-dependency
branch
from
July 21, 2026 10:34
d814cae to
11023d4
Compare
rob-ghost
changed the title
Boot builds the custom fields services before the members service, so the exporter cannot observe them as absent. The module-level require plus the warn-and-degrade branch guarded a state that cannot occur, so the exporter is now a factory taking the definitions and values services, constructed in the members service's init() alongside the other late-resolved members. The labs flag alone decides whether custom field columns are included, which is what it was always meant to decide. The Admin API endpoint still unwraps definitions with a non-null assertion, because endpoint modules are constructed at import time and cannot take an injected instance without a much larger change. No behaviour change either way on the flag.
rob-ghost
force-pushed
the
chore/require-custom-fields-service-dependency
branch
from
July 21, 2026 11:23
f5cb166 to
d7aec80
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The custom fields services are exported as module-level bindings that are undefined until
init()runs. Their consumers in the members service each invented a way to cope with that: the member create path checked whether the request body carried values before asking the service anything, and the CSV exporter warned and exported without custom field columns when the definitions service was absent.Neither state can occur. Boot builds the custom fields services before the members service, so by the time either consumer exists the dependency is resolved. Both were guarding against something the dependency graph already rules out, and the guards obscured that.
Solution
Both consumers now receive the services they need rather than reaching for the module and hoping. The members service is handed the values service at construction, where boot order is expressed; the exporter is a factory built during the members service's init, alongside the other members that are resolved there rather than required statically. With the dependency genuinely present, the guards go, and the labs flag is left as the only thing deciding whether custom fields participate.
Fixtures now build these services the way boot does. One of them was constructing a members service that could not exist in a running Ghost, which is what made the guard look necessary in the first place. The hand-written type describing the values service is replaced by the real exported type, which cannot drift from it.
The Admin API endpoint still unwraps the definitions service with a non-null assertion. Endpoint modules are constructed at import time and cannot take an injected instance without a much larger change, so that one is left as a known case.
No request or export changes behaviour.