Skip to content

Backoffice: Fix core circular import (fetchAllPages) breaking the test check - #23133

Merged
leekelleher merged 1 commit into
v17/devfrom
v17/bugfix/fetch-all-pages-circular-import
Jun 16, 2026
Merged

Backoffice: Fix core circular import (fetchAllPages) breaking the test check#23133
leekelleher merged 1 commit into
v17/devfrom
v17/bugfix/fetch-all-pages-circular-import

Conversation

@iOvergaard

Copy link
Copy Markdown
Contributor

What and why

The test CI job is currently failing on every open PR with:

Error: Bidirectional imports found in 17 modules. 1 more than the threshold.

check:module-dependencies allows 16 bidirectional core-module imports; a recent merge pushed it to 17.

Root cause

#22765 added the offset pagination helper fetchAllPages under @umbraco-cms/backoffice/utils. Its contract is expressed entirely in repository-owned types — UmbDataSourceResponse<UmbPagedModel<T>> (declared in packages/core/repository/). That made the utils module import the repository module, while repository already imports utils → a new repository ↔ utils bidirectional pair (the 17th).

Fix

Move the helper (and its test) into the repository module, which legitimately owns those types, and export it from @umbraco-cms/backoffice/repository. The sole consumer — UmbLanguageCollectionRepository — already imports from that module, so its import just moves from @umbraco-cms/backoffice/utils to @umbraco-cms/backoffice/repository. No type changes, no behaviour change.

Core bidirectional imports are back to 16.

Verification

  • node ./devops/module-dependencies/index.js → exits 0 (Core 16/16, Packages 14/14, illegal 5/5, self 0).
  • tsc (npm run compile) → 0 errors (confirms no other consumer's import broke).
  • Relocated unit test → 8/8 pass.

Unblocks the test check on all open PRs (e.g. #23094).

…core circular import

#22765 added the offset pagination helper `fetchAllPages` under
`@umbraco-cms/backoffice/utils`, but its contract is expressed entirely in
repository-owned types (`UmbDataSourceResponse<UmbPagedModel<T>>`). That made
`utils` import `repository` while `repository` already imports `utils`,
introducing a 17th core bidirectional module import and tripping
`check:module-dependencies` (threshold 16) — failing the `test` job on every
open PR.

Relocate the helper (and its test) into the `repository` module, which
legitimately owns those types, and export it from
`@umbraco-cms/backoffice/repository`. The sole consumer
(UmbLanguageCollectionRepository) already imports from that module. Core
bidirectional imports are back to 16.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings June 16, 2026 07:37
@claude

claude Bot commented Jun 16, 2026

Copy link
Copy Markdown

Claude finished @iOvergaard's task in 3m 16s —— View job


PR Review

Target: origin/v17/dev · Based on commit: 55476fb8d83abc1db613f869fe47fd536933e8a1
Classified as: Refactor

Moves fetchAllPages (and its companion type UmbOffsetPageFetcher) from @umbraco-cms/backoffice/utils into @umbraco-cms/backoffice/repository to break a bidirectional circular import between the two core modules, restoring the check:module-dependencies threshold from 17 back to 16.

  • Modified public API: fetchAllPages and UmbOffsetPageFetcher — removed from @umbraco-cms/backoffice/utils, added to @umbraco-cms/backoffice/repository
  • Affected implementations (outside this PR): Any external plugin that previously imported fetchAllPages or UmbOffsetPageFetcher from @umbraco-cms/backoffice/utils

Important

  • src/Umbraco.Web.UI.Client/src/packages/core/utils/pagination/offset/index.ts:1: Both fetchAllPages and UmbOffsetPageFetcher were previously reachable via the @umbraco-cms/backoffice/utils public subpath export (package.json./utilscore/utils/index.tspagination/index.tsoffset/index.ts). Removing them from that chain without a deprecation shim is a breaking change per docs/deprecation.md.

    Worth noting: the only way to maintain backward compatibility here would be to re-export from utils back to repository, which would re-introduce the circular import this PR is fixing. That makes a clean shim impossible, so the tradeoff is intentional — but it should be called out explicitly.

    Suggestion: Add a note in the PR description (and ideally a changelog entry) that this is a known breaking change for any consumer who imported these from utils, and confirm whether a nightly/pre-release was published containing the utils export before this fix landed. If no release was cut, a simple sentence in the PR body clarifying "no published release contained the utils export" is sufficient to close the loop.


Suggestions

  • src/Umbraco.Web.UI.Client/src/packages/core/repository/pagination/index.ts:1: The new barrel only re-exports fetch-all-pages.function.js. That's correct for now, but if other pagination helpers (e.g. cursor-based) are added to repository in the future they'll go here — the naming pagination/ implies this is the right home. No action required; just noting the intent is clear and well-structured.

Approved with Suggestions for improvement

The architectural fix is correct — fetchAllPages genuinely belongs in repository since it is typed entirely in terms of repository-owned interfaces. The circular import removal is the right call, the sole consumer is updated, tests are preserved, and the diff is minimal. The only open question is whether the removal from utils counts as an API break in practice (given how recently it was added), which the maintainers should confirm before merging.

Labels applied: area/frontend, category/refactor

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR resolves a new core-module bidirectional import introduced by fetchAllPages by relocating the helper (and its unit test) from the utils module into the repository module, and then re-exporting it from @umbraco-cms/backoffice/repository. This removes the utils → repository dependency while keeping the consumer (UmbLanguageCollectionRepository) functioning with the same behavior.

Changes:

  • Moved fetchAllPages (+ test) into packages/core/repository/pagination/ and exported it via the repository barrel.
  • Updated UmbLanguageCollectionRepository to import fetchAllPages from @umbraco-cms/backoffice/repository.
  • Removed the fetchAllPages export from packages/core/utils/pagination/offset/index.ts to prevent the circular dependency.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/Umbraco.Web.UI.Client/src/packages/language/collection/repository/language-collection.repository.ts Switches fetchAllPages import to the repository module (removes utils↔repository circular pair).
src/Umbraco.Web.UI.Client/src/packages/core/utils/pagination/offset/index.ts Stops exporting fetchAllPages from utils’ offset pagination barrel.
src/Umbraco.Web.UI.Client/src/packages/core/repository/pagination/index.ts New barrel export for repository pagination helpers.
src/Umbraco.Web.UI.Client/src/packages/core/repository/pagination/fetch-all-pages.function.ts Relocated helper; now uses repository-local type imports to avoid cross-module dependency.
src/Umbraco.Web.UI.Client/src/packages/core/repository/pagination/fetch-all-pages.function.test.ts Relocated unit test alongside the helper under repository/pagination.
src/Umbraco.Web.UI.Client/src/packages/core/repository/index.ts Re-exports the new repository pagination barrel so consumers can import from @umbraco-cms/backoffice/repository.

@leekelleher
leekelleher merged commit fb8c3b1 into v17/dev Jun 16, 2026
33 checks passed
@leekelleher
leekelleher deleted the v17/bugfix/fetch-all-pages-circular-import branch June 16, 2026 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants