fix(core): Export and register Province entity and ProvinceService#4857
Conversation
Province and ProvinceService are documented as public API but were not exported from @vendure/core, and ProvinceService was never registered as a provider, so it could not be injected. Export the entity, the service and ProvinceEvent (matching the existing Country wiring) and register ProvinceService in the service module. Fixes vendurehq#4792
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughCompact Summary: This change adds three barrel re-exports in packages/core (entity, event-bus, and service index files) to expose the Province entity, ProvinceEvent, and ProvinceService from the public API. Additionally, service.module.ts is updated to import ProvinceService and include it in the services array, registering it as a provider and export within ServiceCoreModule. Changes
Related Issues: Related PRs: None identified Suggested Labels: enhancement, core Suggested Reviewers: None specified Poem 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
michaelbromley
left a comment
There was a problem hiding this comment.
Hi Martin,
Thanks for this fix - adding the exports is probably all that is needed here because the e2e tests are not "true" e2e tests in the sense that all the other e2e tests are making Graphql API calls and asserting against the response, in general.
We could have an e2e suite for Province, but it should work via API not direct service instance injection. I'm not even sure if we have any exposed API surface for this however, as Province as a concept was created with future work in mind (potentially supporting nested regions) but we thus far never really did anything meaningful with it.
So for now I'd say you can remove those e2e tests (not something I often say! 😅)
Unless of course we are exposing a graphql interface for Province, in which case adapt the tests to go via the graphql API.
…ovince-entity-and-provinceservice-not-found
Per review feedback (vendurehq#4857): Vendure e2e tests assert against the GraphQL API, but Province has no exposed API surface, so these service-injection tests don't fit. The export/registration fix is retained. Relates to vendurehq#4792
|
Thanks Michael! Makes sense — there's no GraphQL surface for Province (no |
…nceservice-not-found
Reviewed this myself in conjunction with Nigel, and Martin properly implemented the solution and your changes. Approving and moving through after CI pass
Summary
Province(entity) andProvinceServiceare documented as public API (docs.vendure.io has pages for both) but importing them fails:Fixes #4792
Root cause
Province(a@ChildEntityofRegion, single-table inheritance) andProvinceService(a complete service:findAll/findOne/create/update/delete, already publishingProvinceEvent) both exist, but:entity/index.tsexportedregion/country.entitybut notregion/province.entity.service/index.tsexportedcountry.servicebut notprovince.service.event-bus/index.tsexportedcountry-eventbut notprovince-event.ProvinceServicewas not inservice.module.ts'sservicesarray, so it was never registered as a provider — even if imported, it could not be injected.In a built
@vendure/core,Country/CountryServiceresolve to functions whileProvince/ProvinceServicewereundefined. This is an oversight from the original "Regions & Provinces" work (the Country counterparts were all wired up; the Province ones were missed), not an intentional private API.The change (mirrors the existing
Countrywiring)entity/index.ts— exportregion/province.entityservice/index.ts— exportservices/province.serviceevent-bus/index.ts— exportevents/province-event(so consumers injectingProvinceServicecan subscribe to the events it emits)service.module.ts— importProvinceServiceand add it to theservicesarray (which feeds bothprovidersandexports), making it injectableProvinceuses single-table inheritance, so nodbConnectionOptions.entitiesregistration is needed.Test plan
New
packages/core/e2e/province.e2e-spec.ts:Province,ProvinceServiceandProvinceEventare exported from@vendure/core.server.app.get(ProvinceService)is injectable (DI registration).create→findOne→findAll→delete) and asserts the emittedProvinceEvents are['created', 'deleted'].Verified: fails on
master(Nest could not find given element—ProvinceServiceisn't a provider; the imports resolve toundefined), passes on this branch. Siblingcountry.e2e(6 tests) still green; coretscbuild + ESLint clean.Out of scope (follow-up)
The Admin API schema (
province.api.graphql) declaresprovinces/province/createProvince/updateProvince/deleteProvince, but there is noProvinceResolverregistered (onlyCountryResolver) — those GraphQL operations have never had a backing resolver. That is a pre-existing gap from the same original commit and is independent of this programmatic-export fix; it deserves its own issue. This PR restores the documented programmatic (@vendure/core) surface only.Note: docs generation walks source
@docsCategoryannotations, not the index barrels, which is whyProvinceServicehad a published doc page despite being unexported — so no docs change is required.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.