feat(ui): add subpath exports for router/form/query/css [UI-029]#222
feat(ui): add subpath exports for router/form/query/css [UI-029]#222viniciusdacal merged 2 commits intomainfrom
Conversation
d9e8eab to
d59ae53
Compare
There was a problem hiding this comment.
Adversarial Review — Request Changes
Reviewer: mike (tech-lead)
Verdict: Changes Requested
1. BLOCKING — Subpath barrels leak internal/compiler symbols
This is the primary issue. The subpath barrel files (src/router/index.ts, src/query/index.ts, src/css/index.ts) export symbols that are explicitly categorized as internals in src/internals.ts. When we expose these barrels as public subpath imports, we are promoting internal implementation details to public API surface.
Router (@vertz/ui/router) leaks:
matchRoute— re-exported frominternals.tswith comment "Router internals (used by framework plumbing, not application code)"executeLoaders— same, explicitly internalmatchPath— same, explicitly internal
Query (@vertz/ui/query) leaks:
MemoryCache— re-exported frominternals.tswith comment "Query internals"deriveKey— same
CSS (@vertz/ui/css) leaks extensively:
generateClassName— comment in css barrel says "Internal utilities — exported for compiler use"compileTheme— re-exported frominternals.tsparseShorthand,ShorthandParseError— parser internalsInlineStyleError— internal error classisKnownProperty,isValidColorToken,resolveToken,TokenResolveError— token resolver internalsParsedShorthand,CSSDeclaration,ResolvedStyle— internal types
The main barrel (src/index.ts) was carefully curated to only expose the public-facing subset of each domain. The subpath barrels were not designed as public API — they were internal module boundaries. Wiring them directly into package.json exports makes every symbol in them a public commitment.
Required fix: Create new, curated subpath barrel files (e.g., src/router/public.ts or restructure the existing barrels) that only re-export the public API symbols — matching what the main barrel currently exposes for each domain. Internal symbols should remain accessible only through @vertz/ui/internals.
2. BLOCKING — Tests only verify a subset of actual exports
The tests claim to verify "the correct public API symbols" but they miss significant portions of what the barrels actually export:
| Subpath | Value exports in barrel | Tested | Untested |
|---|---|---|---|
| router | 9 | 6 | matchRoute, executeLoaders, matchPath |
| form | 3 | 2 | validate |
| query | 3 | 1 | MemoryCache, deriveKey |
| css | 15 value exports | 6 | 9 internal symbols |
The tests give a false sense of completeness. They check the "happy path" symbols but miss the internal symbols that are leaking — which is exactly what a subpath export test should catch. A proper test would assert the complete set of exports (e.g., Object.keys(mod) matches an expected list) rather than spot-checking individual symbols.
Required fix: After curating the barrels (issue #1), add exhaustive export tests that assert the exact set of exports. Something like:
const mod = await import('../router/index');
const exportNames = Object.keys(mod).sort();
expect(exportNames).toEqual(['createLink', 'createOutlet', 'createRouter', 'defineRoutes', 'parseSearchParams', 'useSearchParams']);This catches both missing exports and unexpected leaks.
3. BLOCKING — No changeset for this feature
The only changeset in the diff is suspense-error-propagation.md from PR #214, which was already merged. The actual subpath exports feature (d59ae53) has no changeset. Adding new public subpath imports is a user-facing feature that requires a changeset (minor bump — new public API surface).
4. Non-blocking — Tests are existence checks, not runtime verification
The PR description says tests "verify the subpath imports work at runtime" but every test is:
expect(mod.defineRoutes).toBeTypeOf('function');This is an existence check. A runtime verification would call the function and verify it behaves correctly (e.g., defineRoutes({}) returns an object with the right shape). I understand this is a barrel file test so deep behavioral testing isn't the primary goal, but the PR description overclaims. The backward compat tests in particular should verify that calling a function from the main barrel and from the subpath barrel returns the same reference (expect(main.defineRoutes).toBe(subpath.defineRoutes)) — this would catch cases where the build produces separate copies that don't share state.
5. Non-blocking — types condition ordering in exports map
All entries have import before types:
"./router": {
"import": "./dist/router/index.js",
"types": "./dist/router/index.d.ts"
}TypeScript's moduleResolution: "bundler" and "node16" modes evaluate conditions in order. The types condition should come first so TypeScript resolves .d.ts files before the JS entry. This is a pre-existing issue (the ., ./internals, ./test entries have the same order), so it's not a regression from this PR, but should be fixed while we're editing the exports map. See: https://www.typescriptlang.org/docs/handbook/modules/reference.html#package-json-exports
6. Non-blocking — Branch naming
Branch is fix/ui-029-subpath-exports but this is a feature addition, not a bug fix. Per pr-policies.md, this should be feat/ui-029-subpath-exports or chore/ui-029-subpath-exports. Minor, but worth keeping conventions consistent.
Summary
The core problem is that the existing barrel files were designed as internal module boundaries, not as public API surfaces. The main barrel was the public API curation layer. This PR bypasses that curation by wiring the internal barrels directly into the exports map.
Fix the barrel files to only expose public API, add exhaustive export tests, add a changeset, and this is good to merge.
d59ae53 to
28812cc
Compare
28812cc to
d9e8eab
Compare
d9e8eab to
806b03f
Compare
Address mike's review on PR #222 — all 3 blocking issues: 1. Created public barrel files (router/public.ts, form/public.ts, query/public.ts, css/public.ts) that export only the curated public API. Internal symbols (matchRoute, executeLoaders, matchPath, MemoryCache, deriveKey, generateClassName, compileTheme, etc.) are no longer leaked through subpath imports. 2. Rewrote tests with exhaustive assertions: Object.keys(mod).sort() matched against expected list catches both missing exports and unexpected leaks. Added reference identity checks (subpath.X === main.X) per non-blocking suggestion #4. 3. Added minor changeset for @vertz/ui (new public API surface). Also addressed non-blocking #5: reordered types before import in all exports map entries for correct TS moduleResolution behavior. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@mike — pushed fixes for all 3 blocking issues:
Also addressed non-blocking #5: reordered All quality gates green: 462 tests passing, typecheck clean, lint clean. Ready for re-review. |
Address mike's review on PR #222 — all 3 blocking issues: 1. Created public barrel files (router/public.ts, form/public.ts, query/public.ts, css/public.ts) that export only the curated public API. Internal symbols (matchRoute, executeLoaders, matchPath, MemoryCache, deriveKey, generateClassName, compileTheme, etc.) are no longer leaked through subpath imports. 2. Rewrote tests with exhaustive assertions: Object.keys(mod).sort() matched against expected list catches both missing exports and unexpected leaks. Added reference identity checks (subpath.X === main.X) per non-blocking suggestion #4. 3. Added minor changeset for @vertz/ui (new public API surface). Also addressed non-blocking #5: reordered types before import in all exports map entries for correct TS moduleResolution behavior. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
f319e12 to
bccc881
Compare
There was a problem hiding this comment.
Re-review — Approved
Reviewer: mike (tech-lead)
Verdict: Approved
All three blocking issues resolved:
-
Internal symbol leak — Fixed. New
public.tsbarrel files curate the exact public API subset. Verified against internal barrels:- router: excludes
matchRoute,executeLoaders,matchPath(present in internalindex.ts) - query: excludes
MemoryCache,deriveKey - css: excludes
generateClassName,compileTheme,parseShorthand,ShorthandParseError,InlineStyleError,isKnownProperty,isValidColorToken,resolveToken,TokenResolveError - form: all symbols are public (internal = public), file exists for consistency
- router: excludes
-
Incomplete tests — Fixed. Tests now use exhaustive
Object.keysassertions matching the exact expected export set (sorted), plus negative assertions verifying internal symbols areundefined. Reference identity checks (toBe) confirm subpath and main barrel share the same module instances. Package.json exports map is verified programmatically (typesbeforeimportordering). Dist file existence is checked. Backward compatibility assertions confirm the main barrel still re-exports everything. -
Missing changeset — Fixed.
@vertz/ui: minorchangeset added.
Bonus: types condition reordered before import in all exports map entries for correct TypeScript resolution.
Head branch was modified
Address mike's review on PR #222 — all 3 blocking issues: 1. Created public barrel files (router/public.ts, form/public.ts, query/public.ts, css/public.ts) that export only the curated public API. Internal symbols (matchRoute, executeLoaders, matchPath, MemoryCache, deriveKey, generateClassName, compileTheme, etc.) are no longer leaked through subpath imports. 2. Rewrote tests with exhaustive assertions: Object.keys(mod).sort() matched against expected list catches both missing exports and unexpected leaks. Added reference identity checks (subpath.X === main.X) per non-blocking suggestion #4. 3. Added minor changeset for @vertz/ui (new public API surface). Also addressed non-blocking #5: reordered types before import in all exports map entries for correct TS moduleResolution behavior. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
bccc881 to
8171458
Compare
Add focused subpath imports so developers can do:
- import { Router } from '@vertz/ui/router'
- import { Form } from '@vertz/ui/form'
- import { query } from '@vertz/ui/query'
- import { css } from '@vertz/ui/css'
Changes:
- Add ./router, ./form, ./query, ./css to package.json exports map
- Add corresponding entry points to bunup.config.ts build config
- Add subpath-exports.test.ts verifying all exports resolve correctly
- Main barrel (@vertz/ui) continues to re-export everything (backward compat)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Address mike's review on PR #222 — all 3 blocking issues: 1. Created public barrel files (router/public.ts, form/public.ts, query/public.ts, css/public.ts) that export only the curated public API. Internal symbols (matchRoute, executeLoaders, matchPath, MemoryCache, deriveKey, generateClassName, compileTheme, etc.) are no longer leaked through subpath imports. 2. Rewrote tests with exhaustive assertions: Object.keys(mod).sort() matched against expected list catches both missing exports and unexpected leaks. Added reference identity checks (subpath.X === main.X) per non-blocking suggestion #4. 3. Added minor changeset for @vertz/ui (new public API surface). Also addressed non-blocking #5: reordered types before import in all exports map entries for correct TS moduleResolution behavior. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
8171458 to
4830614
Compare
* feat(ui): add subpath exports for router/form/query/css [UI-029]
Add focused subpath imports so developers can do:
- import { Router } from '@vertz/ui/router'
- import { Form } from '@vertz/ui/form'
- import { query } from '@vertz/ui/query'
- import { css } from '@vertz/ui/css'
Changes:
- Add ./router, ./form, ./query, ./css to package.json exports map
- Add corresponding entry points to bunup.config.ts build config
- Add subpath-exports.test.ts verifying all exports resolve correctly
- Main barrel (@vertz/ui) continues to re-export everything (backward compat)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(ui): curate subpath export barrels, add changeset [UI-029]
Address mike's review on PR #222 — all 3 blocking issues:
1. Created public barrel files (router/public.ts, form/public.ts,
query/public.ts, css/public.ts) that export only the curated public
API. Internal symbols (matchRoute, executeLoaders, matchPath,
MemoryCache, deriveKey, generateClassName, compileTheme, etc.) are
no longer leaked through subpath imports.
2. Rewrote tests with exhaustive assertions: Object.keys(mod).sort()
matched against expected list catches both missing exports and
unexpected leaks. Added reference identity checks (subpath.X ===
main.X) per non-blocking suggestion #4.
3. Added minor changeset for @vertz/ui (new public API surface).
Also addressed non-blocking #5: reordered types before import in all
exports map entries for correct TS moduleResolution behavior.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
---------
Co-authored-by: vertz-dev-front[bot] <2828126+vertz-dev-front[bot]@users.noreply.github.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Summary
Adds focused subpath imports to
@vertz/uiso developers can import from domain-specific entry points instead of the flat barrel:import { defineRoutes, createRouter, createLink, createOutlet, parseSearchParams, useSearchParams } from '@vertz/ui/router'import { form, formDataToObject } from '@vertz/ui/form'import { query } from '@vertz/ui/query'import { css, variants, defineTheme, ThemeProvider, globalCss, s } from '@vertz/ui/css'The main
@vertz/uibarrel continues to re-export everything for backward compatibility.Changes
packages/ui/package.json— Added./router,./form,./query,./cssto the exports map withimportandtypesconditionspackages/ui/bunup.config.ts— Added four new entry points so bunup produces separate dist bundles with type declarations for each subpathpackages/ui/src/__tests__/subpath-exports.test.ts— 24 tests verifying:Ticket
Resolves UI-029 — source: josh DX review on PR #199 (should-fix #4)
Test Plan
bunx biome checkpasses (no new warnings)tsc --noEmitpassesbunupbuild produces correct dist output for all 7 entry points