Skip to content

fix: 🐛 restore TypeScript API version of generate-exports script lost in merge conflict resolution#879

Merged
punkbit merged 9 commits intomainfrom
fix/restore-changes-lost-in-PR-resolution-generate-exports-restored-typeScript-api
Mar 6, 2026
Merged

fix: 🐛 restore TypeScript API version of generate-exports script lost in merge conflict resolution#879
punkbit merged 9 commits intomainfrom
fix/restore-changes-lost-in-PR-resolution-generate-exports-restored-typeScript-api

Conversation

@punkbit
Copy link
Collaborator

@punkbit punkbit commented Mar 6, 2026

Why?

Restores the correct TypeScript API-based generate-exports script that was lost during a bad merge conflict resolution, related documentation and changeset.

💡 Relates to #878

How?

  • Restored TypeScript API version using ts.createSourceFile() to parse src/index.ts
  • Added check to only include components with an index.ts barrel export
  • Added warning output for skipped paths

Preview?

Screenshot 2026-03-06 at 15 53 13

@changeset-bot
Copy link

changeset-bot bot commented Mar 6, 2026

⚠️ No Changeset found

Latest commit: d9036d2

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes changesets to release 1 package
Name Type
@clickhouse/click-ui Minor

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions
Copy link

github-actions bot commented Mar 6, 2026

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


PR Review: Restore TypeScript API generate-exports script

  • Gather context
  • Read changed files
  • Provide review feedback

Good restoration work! The switch from dist-based to source-based export generation is a clear improvement — the script no longer requires a build step to run correctly. The use of the TypeScript Compiler API for AST parsing is more precise than directory scanning. Overall looks solid.

💡 Suggestions (non-blocking)

  • .scripts/generate-exports line 60 — minor inefficiency: skipped is computed with candidateComponents.filter(c => !components.includes(c)). Since components is an Array, this is O(n²). A Set would be more efficient, though in practice the number of components is small enough that this won't matter. Acceptable as-is.

  • .scripts/generate-exports — removed error handling: The old version had a try/catch with process.exit(1) on failure (e.g., if src/index.ts doesn't exist). The new version relies on unhandled exceptions which will still produce a non-zero exit, so CI will still fail correctly — just with a less friendly stack trace. Not a blocker.

  • .changeset/fiery-shirts-brush.mdminor bump for a fix: The changeset is typed as minor, but this PR's stated purpose is a fix (restoring a lost script). The minor bump appears intentional since the changeset content describes the removal of previously exposed internal paths (Collapsible, IconWrapper, MiddleTruncator), which is a public API change — so minor is defensible. Worth a quick sanity check by the maintainer.

✅ Everything else looks correct — the TypeScript AST traversal logic, the barrel-export guard (src/components/<name>/index.ts existence check), the output formatting, and the README + docs updates are all accurate and consistent with the stated goal.

@github-actions
Copy link

github-actions bot commented Mar 6, 2026

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


PR Review: Restore TypeScript API generate-exports script

  • Gather context
  • Read changed files
  • Provide review feedback

Great restoration work! The switch from dist/-scanning to TypeScript AST parsing is a clear improvement — no build step required, and exports now accurately reflect src/index.ts as the single source of truth. The additions of ./Input, ./Select, and ./Typography to exports (previously missing) are welcome corrections.

🚨 Must Fix

  • package.json + .changeset/fiery-shirts-brush.mdUndocumented breaking removal: ./GenericMenu is removed from the subpath exports in this PR, but it is not listed in the changeset's "Removed Paths" section. Consumers importing @clickhouse/click-ui/GenericMenu will silently break at upgrade time with no migration guidance. Please add @clickhouse/click-ui/GenericMenu to the removed paths list in the changeset. Fix this →

💡 Suggestions (non-blocking)

  • .scripts/generate-exports line 61skipped is computed via candidateComponents.filter(c => !components.includes(c)). Since components is an Array, this is O(n²). Wrapping components in a Set before filtering would be marginally cleaner, though with the typical number of components this is negligible in practice.

  • .scripts/generate-exports — the typescript package is a devDependency, which is appropriate since this is a dev-only script. Just worth noting that the script will fail if someone runs it in a stripped-down install without dev deps — but that's an expected constraint for codegen scripts.

  • Changeset bump (minor) — Removing public subpath exports (Collapsible, GenericMenu, IconWrapper, MiddleTruncator) is a breaking public API change. minor is on the lighter side for semver; depending on the project's release policy this might warrant major. Worth a quick maintainer sanity check, but defensible given the RC versioning.

✅ Everything else looks correct — the TypeScript AST traversal, barrel-export guard, output formatting, package.json generate:exports script update (removing the unnecessary yarn build && prefix), and README/docs updates are all accurate and consistent with the stated goal.

@github-actions
Copy link

github-actions bot commented Mar 6, 2026

  • .changeset/fiery-shirts-brush.md./GenericMenu is removed from package.json subpath exports in this PR (visible in the diff) but is not listed in the "Removed Paths" section of the changeset. Consumers importing @clickhouse/click-ui/GenericMenu will break silently on upgrade with no migration guidance. Please add it alongside Collapsible, IconWrapper, and MiddleTruncator. Fix this →

The GenerIcMenu shouldn't be exposed, its yet to be consumed. Ignore.

  • Changeset bump (minor) — Removing Collapsible, GenericMenu, IconWrapper, and MiddleTruncator as public subpath exports is a breaking public API change. minor is on the lighter side for semver; worth a maintainer sanity check depending on the project's release policy. Defensible given the RC versioning.

These shouldn't be exposed Collapsible, GenericMenu, IconWrapper, and MiddleTruncator, its yet to be consumed. Ignore.

@github-actions
Copy link

github-actions bot commented Mar 6, 2026

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


PR Review: Restore TypeScript API generate-exports script

  • Gather context
  • Read changed files
  • Provide review feedback

Solid restoration work! The switch to TypeScript AST parsing is a clear improvement over the previous dist-scanning approach — no build step required, and src/index.ts is now the authoritative single source of truth. The script is clean, well-structured, and handles edge cases correctly.

✅ Everything looks correct:

  • .scripts/generate-exports — AST traversal logic is correct. ts.createSourceFile() with true for setParentNodes is the right call. The barrel-export guard (src/components/<name>/index.ts existence check) is a sensible safeguard. The try/catch with process.exit(1) restores proper CI failure behavior. The Set-based skipped computation on line 61-62 is efficient.

  • package.jsongenerate:exports script — Removing yarn build && prefix is correct; the script now reads from src/ directly so a prior build is no longer needed.

  • package.json — exports map./Input, ./Select, ./Typography are now correctly included (previously missing). The removals (Collapsible, IconWrapper, MiddleTruncator, GenericMenu) are confirmed intentional by the author — internal implementation details not ready for public consumption.

  • .changeset/fiery-shirts-brush.md — Well-documented. The minor bump is defensible given the public API surface changes (removed subpath exports). The "How to use" section with the src/index.ts example is a helpful addition for maintainers.

  • README.md — Updated docs correctly reflect the new source-based approach and link to the generate:exports script.

💡 One minor note (non-blocking): The script uses import ts from 'typescript' as a default import, which works because TypeScript's package exports support both CJS and ESM. Worth noting for anyone who might wonder — this is fine as-is since package.json has "type": "module" and typescript ships compatible dual exports.

✅ Looks great — nice work restoring this! No blockers.

@workflow-authentication-public
Copy link
Contributor

📚 Storybook Preview Deployed

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

Built from commit: 94054dc8145413a37470937399c50207180fd110

@punkbit punkbit merged commit ed46475 into main Mar 6, 2026
8 checks passed
@punkbit punkbit deleted the fix/restore-changes-lost-in-PR-resolution-generate-exports-restored-typeScript-api branch March 6, 2026 18:48
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