Skip to content

chore: 🤖 public api as exports source-of-truth#845

Merged
punkbit merged 282 commits intomainfrom
chore/public-api-as-exports-generator-source-of-truth
Mar 6, 2026
Merged

chore: 🤖 public api as exports source-of-truth#845
punkbit merged 282 commits intomainfrom
chore/public-api-as-exports-generator-source-of-truth

Conversation

@punkbit
Copy link
Collaborator

@punkbit punkbit commented Feb 25, 2026

Why?

The team should have full control over the Public API to manage which resources are available for use in consumer applications. Previously, consumer applications had unrestricted access to internal resources, which is undesirable.

For example, third-party APIs like the primitive components provided by Radix UI were directly exposed, meaning that if those primitives were ever swapped out, any consumer applications depending on them would break due to tight coupling.

With these changes in place, core maintainers can now manage the Public API through a clear and friendlier interface.

⚠️ WARNING: Depends on #841, which should be merged first
🤖 On #841 merge, change base branch to main

⚠️ This has leaked through #841 possible during merging. So here we're just looking into the docs, changesets and smaller tweaks over the original work.

How?

  • Uses the fs.readdirSync to traverse the main exports file (single source-of-truth)
  • Modifies the generate-exports to include the fs.readdirSync lookup and traversal
  • Modifies the generate-exports to include a summary and optional user hint to commit changes (if available)
  • Provides documentation for Public API management

Preview?

demo-generate-exports

Base automatically changed from fix/circular-dependencies-first-pass to main March 6, 2026 13:25
@github-actions
Copy link

github-actions bot commented Mar 6, 2026

Claude finished @punkbit's task in 1m 47s —— View job


PR Review: chore: 🤖 public api as exports source-of-truth

  • Gathering context
  • Reading changed files
  • Analyzing changes
  • Posting review

Good direction overall — naming src/index.ts as the single source of truth is a clear improvement, and the documentation additions in README and the changeset are well-written. The removal of internal/primitive exports (Collapsible, IconWrapper, MiddleTruncator, etc.) from the exports map is the right call.

A couple of things need attention before merge:

🚨 Must Fix

  • generate:exports reads dist/, not src/index.ts: The README and changeset both state "The exports map is auto-generated from the public API defined in src/index.ts", but the script at .scripts/generate-exports:9 does fs.readdirSync(componentsDir) where componentsDir = 'dist/esm/components'. It lists every component directory that was compiled, not the subset exported from src/index.ts. This means the claim that src/index.ts is the source of truth is incorrect in practice — dist/ is. Any component directory that exists in dist/ (regardless of whether it's in src/index.ts) will get a component-level export entry. Fix this →

  • Build step missing from documented workflow: Both the README (README.md:349) and changeset (.changeset/fiery-shirts-brush.md:27) instruct maintainers to edit src/index.ts and then run yarn generate:exports. But since the script reads from dist/, running it without a prior yarn build will either fail or silently produce results based on stale build output. The workflow should be yarn build && yarn generate:exports. Fix this →

💡 Suggestions (non-blocking)

  • Off-by-one in summary log: .scripts/generate-exports:45 logs Object.keys(exports).length - 2 but the exports object has only one non-component entry (.), so it should be - 1. Fix this →

  • Radix UI types still exposed: src/index.ts:298-301 still exports ContextMenuProps, DialogProps, DialogTriggerProps, HoverCardProps, and PopoverProps directly from @radix-ui/*. There's a comment right above it saying // TODO: These should NOT be exposed. If one of the PR goals is preventing tight coupling to Radix primitives, this is the remaining gap. Not blocking if intentional, but worth a follow-up issue to finish the job.

  • linkStyles leaking implementation detail: src/index.ts:157 exports linkStyles (a styled-component) with the comment // TODO: This linkStyles require investigation. This is an internal styling primitive leaking into the public API. Worth tracking for cleanup.

  • Changeset should enumerate removed paths: For consumers who were deep-importing (e.g. @clickhouse/click-ui/Collapsible, @clickhouse/click-ui/IconWrapper, @clickhouse/click-ui/MiddleTruncator), listing the removed paths explicitly in the changeset would help them audit their codebases. The minor bump is reasonable given these were implementation details.

🙌 Solid work on the documentation side — the Public API section in README and the backwards-compat examples are clear and useful.

@github-actions
Copy link

github-actions bot commented Mar 6, 2026

Claude finished @punkbit's task in 1m 41s —— View job


PR Review: chore: 🤖 public api as exports source-of-truth

  • Gathering context
  • Reading changed files
  • Analyzing changes
  • Posting review

Solid cleanup — replacing export * from './components' with an explicit, curated src/index.ts is exactly the right approach for a component library that wants to control its public surface. The documentation additions (README Public API section, changeset migration notes) are thorough and well-written. Good iteration from previous review rounds.

🚨 Must Fix

  • Off-by-one in summary log (still unresolved from previous reviews): .scripts/generate-exports:45 logs Object.keys(exports).length - 2 but the exports object has exactly one non-component entry ('.'), so the component count is off by one. Should be - 1. Fix this →

💡 Suggestions (non-blocking)

  • Radix UI types still in public API: src/index.ts:298-301 exports ContextMenuProps, DialogProps, DialogTriggerProps, HoverCardProps, PopoverProps directly from @radix-ui/* packages with a // TODO: These should NOT be exposed comment. Since one goal of this PR is preventing tight coupling to Radix primitives, this is the remaining gap. Worth a follow-up issue to replace these with Click-UI–owned wrapper types.

  • linkStyles in public API: src/index.ts:157 exports linkStyles with // TODO: This linkStyles require investigation. This styled-component is an implementation detail leaking through. Good candidate for cleanup once investigated.

  • Removed paths not fully enumerated in changeset: fiery-shirts-brush.md doesn't list ./Collapsible, ./IconWrapper, ./MiddleTruncator as removed subpath exports (the fast-drinks-argue.md changeset covers some others like CrossButton, EmptyButton, GridCenter). Consumers deep-importing these paths would benefit from knowing what was removed.

✅ Previous feedback addressed

  • Build step is now chained in the npm script: "generate:exports": "yarn build && .scripts/generate-exports"
  • README includes a clear [!IMPORTANT] callout that the script reads from dist/ and that yarn build is a prerequisite ✓
  • The relationship between src/index.ts (source of truth) → dist/ (generated artifact) → exports map is now clearly documented ✓

Overall this is in great shape. The one must-fix is a cosmetic log line, not a functional issue. 🙌

@github-actions
Copy link

github-actions bot commented Mar 6, 2026

Claude finished @punkbit's task in 48s —— View job


PR Review: chore: 🤖 public api as exports source-of-truth

  • Gathering context
  • Reading changed files
  • Analyzing changes
  • Posting review

Great cleanup overall — the documentation additions are thorough and well-written, and the [!IMPORTANT] callout clearly explains the dist/ relationship. All major feedback from previous review rounds has been addressed. One small item remains before merge:

🚨 Must Fix

  • Off-by-one in summary log (still unresolved from all three prior reviews): .scripts/generate-exports:45 logs Object.keys(exports).length - 2 but the exports object has exactly one non-component entry ('.'), so the component count is always one less than the actual number of generated entries. Should be - 1. Fix this →

💡 Suggestions (non-blocking)

  • Changeset missing removed paths: fiery-shirts-brush.md doesn't enumerate the subpath exports that were removed (e.g. ./Collapsible, ./IconWrapper, ./MiddleTruncator). Consumers deep-importing those paths would benefit from an explicit list to audit their codebases.

  • Radix UI types still in public API: src/index.ts:298-301 still exports ContextMenuProps, DialogProps, DialogTriggerProps, HoverCardProps, PopoverProps from @radix-ui/* with a // TODO: These should NOT be exposed comment. Worth tracking as a follow-up to finish decoupling from Radix primitives.

✅ Previous feedback addressed

  • Build step is now chained: "generate:exports": "yarn build && .scripts/generate-exports"
  • README includes a [!IMPORTANT] callout explaining the script reads from dist/ and that build is a prerequisite ✓
  • The relationship between src/index.ts (source of truth) → dist/ (generated artifact) → exports map is clearly documented ✓

Solid PR — the public API story is much cleaner and the documentation is genuinely useful for maintainers. 🙌

@github-actions
Copy link

github-actions bot commented Mar 6, 2026

Claude finished @punkbit's task in 39s —— View job


PR Review: chore: 🤖 public api as exports source-of-truth

  • Gathering context
  • Reading changed files
  • Analyzing changes
  • Posting review

All previously flagged must-fix items have been addressed in this revision. Here's the summary:

✅ All previous feedback resolved

  • Off-by-one in summary log fixed: .scripts/generate-exports:45 now correctly uses - 1
  • Build step chained in package.json: "generate:exports": "yarn build && .scripts/generate-exports"
  • README includes [!IMPORTANT] callout explaining the dist/ relationship ✓
  • Changeset enumerates all removed subpath exports (Collapsible, IconWrapper, MiddleTruncator) ✓

💡 Suggestion (non-blocking)

  • Redundant build in docs: README.md:361 shows yarn build && yarn generate:exports, but since generate:exports already chains yarn build internally (per package.json), this runs the build twice. The docs could just say yarn generate:exports — or keep it as-is since it's harmless and makes the dependency explicit.

This PR is in great shape. The public API story is clean, the documentation is thorough, and the tooling is correctly wired up. 🙌

@github-actions
Copy link

github-actions bot commented Mar 6, 2026

Claude finished @punkbit's task in 1m 14s —— View job


PR Review: chore: 🤖 public api as exports source-of-truth

  • Gathering context
  • Reading changed files
  • Analyzing changes
  • Posting review

All feedback from previous review rounds has been addressed. Here's the final summary:

✅ All previous feedback resolved

  • Off-by-one in summary log fixed: .scripts/generate-exports:45 now correctly uses - 1
  • Build step chained in package.json: "generate:exports": "yarn build && .scripts/generate-exports"
  • README includes [!IMPORTANT] callout explaining that the build step runs automatically when calling yarn generate:exports
  • README now shows just yarn generate:exports (no redundant double-build) ✓
  • Changeset enumerates all removed subpath exports (Collapsible, IconWrapper, MiddleTruncator) ✓

💡 Remaining non-blocking notes (carry-forward, no action needed before merge)

  • Radix UI types still in public API: src/index.ts still exports ContextMenuProps, DialogProps, DialogTriggerProps, HoverCardProps, PopoverProps from @radix-ui/* with existing // TODO: These should NOT be exposed comments. Good candidate for a follow-up issue to replace with Click-UI–owned wrapper types.
  • linkStyles in public API: src/index.ts exports linkStyles with a // TODO: This linkStyles require investigation comment. Worth tracking for cleanup once investigated.

This PR is ready to merge (pending the base branch swap from #841 as noted in the PR description). The public API story is clean, the tooling is correctly wired up, and the documentation is thorough and accurate. 🙌

@workflow-authentication-public
Copy link
Contributor

📚 Storybook Preview Deployed

✅ Preview URL: https://click-n3kmh5kfl-clickhouse.vercel.app

Built from commit: 62a3343c6f78126e08f9bc4abaebeab99f3fd1e9

@punkbit
Copy link
Collaborator Author

punkbit commented Mar 6, 2026

⚠️ This has leaked through #841 possible during merging. So here we're just looking into the docs, changesets and smaller tweaks over the original work.

@punkbit punkbit merged commit 7dad1bb into main Mar 6, 2026
9 checks passed
@punkbit punkbit deleted the chore/public-api-as-exports-generator-source-of-truth branch March 6, 2026 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants