- 
          
 - 
                Notifications
    
You must be signed in to change notification settings  - Fork 1.3k
 
fix(start-plugin-core): Remove framework specific packages from being required #5409
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
          
WalkthroughAdds two workspace-scoped peerDependencies to packages/start-plugin-core/package.json and marks them optional via peerDependenciesMeta. No runtime code or control flow changed. Changes
 Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
 Pre-merge checks and finishing touches✅ Passed checks (5 passed)
 ✨ Finishing touches
 🧪 Generate unit tests (beta)
 Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment   | 
    
| 
           View your CI Pipeline Execution ↗ for commit 0ff49af 
 ☁️ Nx Cloud last updated this comment at   | 
    
          More templates
 
 @tanstack/arktype-adapter
 @tanstack/directive-functions-plugin
 @tanstack/eslint-plugin-router
 @tanstack/history
 @tanstack/nitro-v2-vite-plugin
 @tanstack/react-router
 @tanstack/react-router-devtools
 @tanstack/react-router-ssr-query
 @tanstack/react-start
 @tanstack/react-start-client
 @tanstack/react-start-server
 @tanstack/router-cli
 @tanstack/router-core
 @tanstack/router-devtools
 @tanstack/router-devtools-core
 @tanstack/router-generator
 @tanstack/router-plugin
 @tanstack/router-ssr-query-core
 @tanstack/router-utils
 @tanstack/router-vite-plugin
 @tanstack/server-functions-plugin
 @tanstack/solid-router
 @tanstack/solid-router-devtools
 @tanstack/solid-start
 @tanstack/solid-start-client
 @tanstack/solid-start-server
 @tanstack/start-client-core
 @tanstack/start-plugin-core
 @tanstack/start-server-core
 @tanstack/start-static-server-functions
 @tanstack/start-storage-context
 @tanstack/valibot-adapter
 @tanstack/virtual-file-routes
 @tanstack/zod-adapter
 commit:   | 
    
fd4035d    to
    9a3bf43      
    Compare
  
    | 
           i think we should just remove this ...resolveRuntimeFiles({
  package: `@tanstack/${framework}-start-client`,
  files: ['index.js'],
}), | 
    
          
 Is it not needed?  | 
    
| 
           it was an optimization to compile less (unnecessarily), but since we restructured exports etc. this is not doing anything anymore  | 
    
9a3bf43    to
    e413e33      
    Compare
  
    
          
 Alright. PR is now updated.  | 
    
| 
           @schiller-manuel Sweet. I've tested the new release (v1.132.52) and it's working now in my monorepo!  | 
    
Hopefully fixes #5398 part 2
@schiller-manuel thanks for merging #5408. I've now tested the new version, and now I have encountered the next issue:
packages/start-plugin-core/src/start-compiler-plugin/plugin.tsalso contains this snippet of code:I believe this is supposed to be either
react-start-clientorsolid-start-client. Without declaring the dependency, we run into the error:Whichever one is needed must also be declared in dependencies, and it seems like incorrect design to me to add both to dependencies. I think the best way to handle this is using
peerDependencieswithpeerDependenciesMetato declare them as optional. This way sincereact-startandsolid-startalready declare bothstart-plugin-coreand their respect-start-clientpackages as dependencies, theoretically this should cleanly resolve the issue.Do you have an opinion on this? c.c. @tannerlinsley
Summary by CodeRabbit
New Features
Bug Fixes
Refactor