Conversation
|
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
cc67f7c to
0642d87
Compare
| @@ -0,0 +1,52 @@ | |||
| { | |||
| "name": "@ownclouders/web-test-helpers-core", | |||
There was a problem hiding this comment.
web-pkg, web-test-helpers, and design-system formed a dependency cycle (pnpm was already warning about it), which also blocks giving them a cached Nx build target. web-test-helpers-core breaks that cycle: it's the generic test-helper code (mount, stubs, defaultPlugins, etc.) that doesn't depend on anything in the cycle — defaultPlugins gets the real DesignSystem plugin and pinia factory injected via a small registry instead of importing them directly.
web-test-helpers itself is now just a thin facade that re-exports web-test-helpers-core (plus web-pkg/src/testing for the web-pkg-specific mocks) under the same names, so nothing changes for the 17 published web-extensions that depend on it. I kept it this way instead of moving those mocks directly, since that was tried first and turned out to be a silent breaking change for those extensions.
There was a problem hiding this comment.
Hmm, personally, I do not like having another package for this. It's already leading to some confusion the split of the existing packages. Having another one in play and having one just as to re-export stuff feels wrong.
There was a problem hiding this comment.
Fair point — the extra package is gone. Instead of a package that only re-exports, the helpers now sit in the packages that already need them:
@ownclouders/design-system/testing—mount/shallowMount, prop types, and the design system + gettext + router-link-stub plugins. It has to live here: design-system is at the bottom of the workspace graph, so it can't depend on a package that installs it — that was the cycle.web-pkg/src/testing— composes the above and adds casl abilities, mocked pinia stores and the component mocks.@ownclouders/web-test-helpers— a singleexport *of that, unchanged public API, so extensions aren't affected.
Every helper is still defined exactly once (web-pkg extends the design system's plugin list rather than duplicating it), and no package depends on one above it, so dependsOn: ["^target"] in Nx works. Net effect vs. master: no new package, no plugin registry singleton, no .npmrc hoist entries, and web-pkg no longer has a test package in its runtime dependencies.
…out changing the published API
The workspace package graph had one strongly connected component:
{web-pkg, web-test-helpers, design-system}. web-pkg and design-system
depend on web-test-helpers for testing; web-test-helpers depended back
on both (design-system for the real DesignSystem Vue plugin, web-pkg
for types used by its own mocks). pnpm already warned about this
("cyclic workspace dependencies"); it also blocks giving these
packages a cached Nx build target with dependsOn: ["^build"].
An earlier attempt at this fix moved the web-pkg-coupled mocks out of
@ownclouders/web-test-helpers and into a new @ownclouders/web-pkg/src
subpath, removing them from web-test-helpers' public exports. That is
a breaking change for the 17 published web-extensions packages that
depend on @ownclouders/web-test-helpers (^12.5.0) - and a silent one,
since defaultPlugins() would only throw at test-run time.
This does the same structural fix without touching the public API:
- New @ownclouders/web-test-helpers-core: everything generic (mount,
shallowMount, getComposableWrapper, defaultStubs, httpResponse,
createRouter, writable, ...) plus an injectable defaultPlugins that
reads the real DesignSystem plugin and pinia store factory from a
small registry instead of importing them. Depends on nothing in the
cycle.
- web-pkg/src/testing: the mocks that need web-pkg's own types
(defaultComponentMocks, createMockStore/PiniaMockOptions,
useAppDefaultsMock, useGetMatchingSpaceMock) move here, published as
a new "./src/testing" subpath so the facade can still reach them.
web-pkg's own specs use this locally; nothing else needed to change.
- @ownclouders/web-test-helpers is now a thin facade: it registers the
real DesignSystem plugin and pinia factory (import side effect) and
re-exports web-test-helpers-core + web-pkg/src/testing under the
exact same names. External consumers see zero difference.
- design-system's and web-pkg's own specs (only ones inside the former
cycle) import straight from web-test-helpers-core / ../testing
instead of the facade; every other package keeps importing
@ownclouders/web-test-helpers unchanged.
- Shared vitest setup (tests/unit/config/vitest.init.ts) registers the
same real plugin/factory for those two packages' own test runs,
since they bypass the facade's registration.
Verified: pnpm install no longer warns about a workspace cycle;
vue-tsc, eslint and prettier all pass; all 16 vitest projects pass
individually (380 files / 3130 tests, 0 failures, matching pre-change
coverage exactly).
Introduces Nx on top of the existing pnpm workspace, no behavior change to any existing script. - Add nx (^23.2.1) as a workspace devDependency - Add nx.json with a targetDefaults entry for test:unit (cache: true) - Ignore .nx/cache and .nx/workspace-data Nx auto-discovers all 25 real projects (24 packages + e2e) from the pnpm workspace with no plugin config needed. Of those, test:unit is the only target with genuine existing per-project granularity today (15/24 packages declare their own script) - lint/check:types/build stay monolithic root scripts for now, per the phased plan. The root package.json intentionally does not become an Nx project: Nx doesn't turn the directory holding nx.json into a project, and there's no source tree of its own for other projects to be "affected by" - root's remaining scripts (build, licenses:check, check:format) stay whole-repo, invoked directly, matching the target model in the design doc. Verified: nx run @ownclouders/web-pkg:test:unit cold (80s, 0/1 cache hit, 159 files passed) then unchanged (116ms, 1/1 cache hit, identical results); a touched file correctly invalidated the cache (0/1 hit). Root lint and check:types still pass clean, untouched.
triggerUpload is a <script setup> binding, not part of the public component instance type, so TS2345 fails vue-tsc --noEmit. Pre-existing, unrelated to the cycle/Nx work in the prior two commits - found while re-verifying the full typecheck.
Registers @nx/vitest's inference plugin so every package with a
vitest.config.ts gets a real, cached "test" target read directly from
its own config - not just the 15/24 packages that happened to already
declare a "test:unit" npm script (Phase 0's target).
- @nx/vite was the wrong package for this (it only infers
build/serve/preview/typecheck targets, never test) - replaced with
@nx/vitest.
- The plugin's default file glob (**/{vite,vitest}.config.*) also
matches vite.config.common.ts (a plain compiler-options module, not
a Vite config) and the root vite.config.ts (the app bundler, not a
test config); loading either as a test config throws and aborts the
whole project graph. Excluded both, plus vite.cern.config.ts for the
same reason.
- defaultBase: "master" - bare `nx affected` defaults to comparing
against a branch named "main"; ocis's default branch is "master",
so without this every affected/dry-run invocation errored with
"ambiguous argument 'main': unknown revision".
- analytics: false - written by the nx CLI itself the first time it
ran interactively; disabling Nx's own usage telemetry, no Nx Cloud
token/URL configured anywhere (matches Phase 0's "local cache only"
decision).
Deliberately NOT changed: the root "test:unit" script. `nx run-many -t
test` runs each project as its own vitest process, writing coverage to
{workspaceRoot}/coverage/{projectRoot} per project - not the single
aggregated lcov report `pnpm test:unit --coverage` produces today.
Nothing currently consumes that aggregate (no codecov config, no
upload step in web-tests.yml), but flipping the script would still be
a real behavior change with no aggregation story yet, so left it
alone. `nx affected -t test` / `nx run-many -t test` are additional
commands, not a replacement, until that's worth solving.
Verified: nx run-many -t test succeeds for all 16 vitest projects
(matches individual project counts from earlier verification exactly);
nx run <project>:test caches correctly (1/1 hit on rerun); nx show
projects --affected --files=... still resolves to the exact same 2
projects as before (web-app-search, web-app-files) - the new plugin
doesn't change affected-detection accuracy; nx affected -t test
--uncommitted correctly scopes to just the 2 dependent projects on an
isolated single-file edit once defaultBase is set.
Also set testMode: "run" on the plugin. Without it, `nx run
<project>:test` uses vitest's default watch mode locally (only
auto-switches to run-once in CI), leaving the terminal waiting for
input (press q to exit) - not what you want from a one-shot command.
"lint": "lint:stylelint" tries to exec a binary literally named "lint:stylelint", which doesn't exist - it needed "npm run"/"pnpm run" to actually invoke the script by that name. Pre-existing, unrelated to any Nx work: the root `lint` script never delegates per-package (it globs the whole workspace directly with eslint), and nothing in CI calls `pnpm --filter design-system run lint` either, so this has never been exercised. Also discovered `lint:stylelint` itself has never successfully run - it's missing the `postcss-scss` dependency stylelint's SCSS custom syntax needs (Cannot find package 'postcss-scss', reproduces identically outside of Nx). That's a separate, deeper pre-existing gap, left alone. Removed the alias entirely rather than fixing it in place: nothing currently calls it, and Nx's per-project lint target (next commit) needs the "lint" name free to create a real ESLint-backed target for this package, same as every other one.
Phase 2a - lint, via @nx/eslint/plugin (correct subpath; the bare package name silently produces zero targets - same gotcha as @nx/vitest before it). Despite there being only one shared root eslint.config.js and no per-package configs, the plugin's glob also matches every package.json, so it still creates a real per-project "lint" target for all 26 projects, each scoped to just that project's files. Root `lint` script untouched. Verified: nx run-many -t lint succeeds for all 26 projects (exit 0 - same "warnings don't fail the run" semantics as the existing root lint); root `pnpm run lint`/`check:format` still pass unchanged. Phase 2b - typecheck, via @nx/vite/plugin's typecheckTargetName (a Vite-specific plugin, distinct from the build/serve/preview targets it also wants to create - those are pointed at unused names, since this repo builds everything through one root vite.config.ts rather than per-package, so auto-generated per-package build/serve targets would be noise, not signal). This one is genuinely partial, matching the spec's own "fragile, spike-gated" flag for this phase: the plugin only creates a typecheck target for projects with their own real vite.config.ts - design-system, web-client, web-pkg, web-test-helpers, web-test-helpers-core (5 of 27). The other ~20 web-app-*/web-runtime projects only have a vitest.config.ts (they're bundled together, not built independently), which this plugin doesn't pick up for typecheck at all. Extending coverage to them would mean either giving each its own vite.config.ts or splitting the shared tsconfig into real TS project references - both bigger, riskier changes not attempted here. Root `pnpm run check:types` (whole-repo vue-tsc, unchanged) stays the one check that covers everything; the 5 new per-project targets are an additive convenience on top, not a replacement. Verified: nx run-many -t typecheck succeeds for all 5 registered projects; a deliberately injected type error in web-pkg/src/index.ts was caught correctly (TS2322, exit 1) and reverted; root `pnpm run check:types` (vue-tsc --noEmit, synchronous, real captured exit code) still passes clean, 0 errors.
… (Phase 3) Adds a `full-run` output to detect-web-changes: true for push to master/stable-* or a `[full-ci]` PR title (the two cases that already always ran everything), false for a normal PR. web-build-test's Lint and Run unit tests steps branch on it - full-run keeps calling `pnpm lint` / `pnpm test:unit --coverage` exactly as before (zero behavior change for those two cases); a normal PR instead runs `nx affected -t lint` / `nx affected -t test --base=<PR base sha>`, scoped to only what the PR could actually break. Two things deliberately NOT switched to `nx affected`, both for reasons already hit while building phases 1/2a/2b: - Check types stays `pnpm check:types` (whole-repo), unconditional, every run. Only 5/27 projects have a real per-project `typecheck` target (needs a project's own vite.config.ts; most web-app-* packages only have a vitest.config.ts) - affected-scoping typecheck would silently skip the other 22. - Run unit tests' full-run path keeps `pnpm test:unit --coverage` producing one aggregated lcov report, not `nx run-many -t test` (which would split it into 16 per-project reports with no merge step - the same gap flagged and deliberately left alone in Phase 1). The affected-scoped PR path has no such expectation to preserve, so it can safely use the per-project reports as-is. Added fetch-depth: 0 to web-build-test's checkout (missing before; needed for `nx affected --base=<sha>` to find that commit). Verified: workflow YAML parses; `nx affected -t lint --base=HEAD` (zero diff) exits 0 with "No tasks were run" rather than failing; `pnpm exec nx affected -t lint --base=<a real prior commit>` (the exact invocation used in the workflow) runs and passes for all 26 projects.
Drops `@ownclouders/web-test-helpers-core` again and keeps the workspace
graph acyclic by layering the helpers onto the packages that already need
them:
design-system/testing mount/shallowMount, prop types, design system +
gettext + router-link stub plugins
web-pkg/src/testing the above, extended with casl abilities, mocked
pinia stores and component mocks
web-test-helpers re-export of web-pkg/src/testing
design-system sits at the bottom of the graph, so it cannot depend on a
package that installs it - hence the generic half lives there. web-pkg
composes those plugins rather than duplicating them, so every helper is
still defined exactly once, and no package depends on one above it.
The public API of `@ownclouders/web-test-helpers` is unchanged, so
external extensions are unaffected. This also removes the testing package
from web-pkg's runtime dependencies, the module-level plugin registry and
the two public-hoist entries in .npmrc.
Two fixes to the published artifacts, both verified by mounting through
the built bundles the way an external extension resolves them:
- keep `@ownclouders/design-system/testing` external in web-pkg's lib
build. rollup matches `external` strings exactly, so the subpath was
inlined, shipping a second copy of the design system and of
@vue/test-utils - components and wrappers were then not identity-equal
to the consumer's own, and gettext injection broke.
- name the web-test-helpers umd output `.cjs`. The package is ESM, so the
`.js` file that publishConfig.exports pointed `require` at could not be
required.
140fbe5 to
e6a8387
Compare
Description
Introduces Nx as the task orchestrator for the
web/pnpm workspace, so lint/typecheck/testonly run against projects actually affected by a change instead of the whole monorepo on every
PR. Adds per-project test, lint, and typecheck targets, breaks a package dependency cycle that
was blocking per-project test targets, and scopes CI to
nx affectedon normal PRs (full,unscoped runs still happen on push and
[full-ci]).The cycle was
{design-system, web-pkg, web-test-helpers}: the mount helpers have to installthe design system (its components are registered globally) and web-pkg's mocked pinia stores,
so the test-helper package was imported by both of the packages it imported. It is now resolved
by layering the helpers onto the packages that already need them, without adding a package:
@ownclouders/design-system/testingmount/shallowMount, prop types, design system + gettext + router-link-stub pluginsweb-pkg/src/testing@ownclouders/web-test-helpersexport *of the abovedesign-system is at the bottom of the workspace graph, so it cannot depend on a package that
installs it — hence the generic half lives there. web-pkg composes those plugins instead of
duplicating them, so each helper is still defined exactly once and no package depends on one
above it. The public API of
@ownclouders/web-test-helpersis unchanged, so externalextensions are unaffected.
Two consequences worth calling out for review:
@pinia/testingandvitest-mock-extendedare nowdependenciesof web-pkg. They are onlyreachable from its
./src/testingentry (never from an app bundle), but they have to bedeclared there rather than in
web-test-helpers, otherwise the published testing entry has aphantom dependency and fails to resolve under a strict pnpm layout.
@ownclouders/web-pkg/src/testing(withsrcin the name) so thatthe same specifier resolves both in the workspace, where web-pkg has no
exportsmap and isconsumed as source, and for published consumers via
publishConfig.exports.Related Issue
Motivation and Context
web/'s CI ran every check against every package on every PR, regardless of what changed.Nx's dependency graph lets CI scope lint/test to only the projects affected by a given diff,
cutting CI time on small PRs while still running everything on master/full-ci.
The dependency cycle had to go with it: Nx tolerates a cycle for
inputs: ["^default"], but anytarget using
dependsOn: ["^target"]fails outright withCould not execute command because the task graph has a circular dependency. That rules out build/typecheck pipelines as long as thecycle exists, so it is fixed here rather than left to bite later.
How Has This Been Tested?
pnpm install --frozen-lockfile, Node 24.15.0 / pnpm 10.33.3nx run-many -t test --skip-nx-cache— 16/16 projects passpnpm test:unit(full root vitest run) — 380 files / 3113 tests pass, unchanged from before the refactorpnpm lint,pnpm check:types(vue-tsc, 0 errors),pnpm check:format,pnpm licenses:check— all pass; 95 lint warnings, all pre-existingpnpm build— succeedsnx show projects --affected --files=<shared config file>— correctly returns all 16 test-bearing projects for changes topnpm-lock.yaml,nx.json,eslint.config.js, and shared vitest setup/stub files (previously returned none, due to an emptysharedGlobals)dependsOn: ["^test"]now builds a task graph; with the cycle restored it fails as above.pnpm installemits no cyclic-workspace warningdist/bundles, resolving every@ownclouders/*specifier viapublishConfig.exportsinstead of workspace source. Asserts that components are identity-equal to the consumer's own@ownclouders/design-systemimport, wrappers are instances of the consumer's own@vue/test-utils,$gettextrenders (singlevue3-gettextcopy), and the mocked pinia stores are shared with the main web-pkg bundle. AllpublishConfig.exportstargets exist for design-system, web-pkg and web-test-helpers@ownclouders/web-test-helpers— 28/28 names still present, two additive (routerLinkStubPlugin,DesignSystemPluginsOptions)Screenshots (if appropriate):
Types of changes
Checklist: