keep file structure for build dist directory with tsdown
#1313
Conversation
🦋 Changeset detectedLatest commit: f08aeb1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| banner: { | ||
| js: '#!/usr/bin/env node', | ||
| }, |
There was a problem hiding this comment.
in which file we need this banner? I think it's better to add directly to its source code
|
Claude finished @dimaMachina's task —— View job Staff Engineer ReviewI've completed my analysis of this PR. Here's my comprehensive feedback as a TypeScript Staff Engineer and System Architect. Review Progress
🎯 Overall Assessment: Strong Architectural ImprovementThis PR represents a thoughtful refactoring that addresses several important architectural concerns:
🔍 Key Changes Analysis1. Build Strategy Transformation ⭐Old approach: Explicit entry points with bundling entry: { index: 'src/index.ts', config: 'src/config.ts' }
noExternal: [/^@inkeep\/agents-core/] // Bundled workspace packagesNew approach: Preserve source structure entry: ['src/**/*.ts', '!**/__tests__', '!**/*.test.ts']
external: ['@inkeep/agents-core']
unbundle: trueImpact: ✅ This change is architecturally sound and follows Node.js best practices for CLI tools. 2. Critical Issue: Missing Banner
|
| ".": "./src/index.ts", | ||
| "./constants/models": "./src/constants/models.ts", | ||
| "./constants/schema-validation": "./src/constants/schema-validation/index.ts", | ||
| "./schema": "./src/db/schema.ts", | ||
| "./db/schema": "./src/db/schema.ts", | ||
| "./auth": "./src/auth/auth.ts", | ||
| "./auth/schema": "./src/auth/auth-schema.ts", | ||
| "./auth/validation": "./src/auth/auth-validation-schemas.ts", | ||
| "./auth/permissions": "./src/auth/permissions.ts", | ||
| "./db/test-client": "./src/db/test-client.ts", | ||
| "./types": "./src/types/index.ts", | ||
| "./validation": "./src/validation/index.ts", | ||
| "./client-exports": "./src/client-exports.ts", | ||
| "./credential-stores": "./src/credential-stores/index.ts", |
There was a problem hiding this comment.
I think we should avoid usage of clean-package.config.json to keep single part of source
| "@better-auth/sso": "^1.4.0", | ||
| "@clack/prompts": "^0.11.0", | ||
| "@inkeep/agents-core": "workspace:^", | ||
| "@nangohq/node": "^0.69.14", |
There was a problem hiding this comment.
Were these dependencies not used?
There was a problem hiding this comment.
| "outputs": ["dist/**", "build/**", ".next/**", "!.next/cache/**"] | ||
| }, | ||
| "dev": { | ||
| "dependsOn": ["^build"], |
There was a problem hiding this comment.
Run "build" task in workspaces I depend on first
| "@inkeep/agents-docs#typecheck": { | ||
| "dependsOn": ["build"], | ||
| "inputs": ["$TURBO_DEFAULT$"], | ||
| "outputs": [".next/**"] | ||
| }, |
There was a problem hiding this comment.
this seems no longer needed
|
@claude review now |
|
Claude finished @dimaMachina's task in 2m 26s —— View job ✅ PR Review CompleteI've completed my review of the changes to the build configuration and dependency management. Here's my comprehensive feedback: Review Checklist:
📋 SummaryThis PR represents a significant architectural shift from bundling to preserving file structure in the build output, along with substantial dependency cleanup. The approach is generally sound, but there's one critical issue that must be addressed before merging. 🚨 Critical IssueMissing Shebang in CLI Entry PointThe removal of the Current State:
Required Fix: Add the shebang directly to #!/usr/bin/env node
import './env'; // Load environment files first (needed by instrumentation)This aligns with @dimaMachina's review comment about adding it to the source file, which is indeed the better approach. ✅ Positive Changes1. Build Strategy Improvements
2. Dependency CleanupThe removal of 16 dependencies from
3. Clean Package RemovalRemoving
|
I think we should keep file structure for build
distdirectory.This PR also
agents-cli@inkeep/agents-coreinagents-cliclean-package,clean-package.config.jsonandpackage.jsonscripts