-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
fix(router-devtools-core): actually bundle solid #5422
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
base: main
Are you sure you want to change the base?
Conversation
WalkthroughAdds bundling of Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer/CI
participant tanstack as tanstackViteConfig
participant Vite as Vite/Rollup
Dev->>tanstack: build using merged config (bundledDeps: ["solid-js","solid-js/web"])
tanstack->>Vite: emit Vite config with bundledDeps
Vite->>Vite: bundle listed deps into output (no externalize)
Vite-->>Dev: produced build artifacts (solid-js imports resolved)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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 |
|
Command | Status | Duration | Result |
---|---|---|---|
nx affected --targets=test:eslint,test:unit,tes... |
❌ Failed | 2m 5s | View ↗ |
nx run-many --target=build --exclude=examples/*... |
❌ Failed | 28s | View ↗ |
☁️ Nx Cloud last updated this comment at 2025-10-13 20:29:49
UTC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/router-devtools-core/vite.config.ts
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/router-devtools-core/vite.config.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: TanStack/router#0
File: AGENTS.md:0-0
Timestamp: 2025-09-23T17:36:12.598Z
Learning: Applies to packages/router-plugin/** : Use unplugin for universal bundler plugins in the router-plugin package
🔇 Additional comments (1)
packages/router-devtools-core/vite.config.ts (1)
18-19
: Good practice documenting the temporary workaround.The TODO comment clearly communicates the intent to refactor this into a cleaner solution within
@tanstack/config/vite
. This aligns well with the PR description's stated follow-up plans.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/router-devtools-core/vite.config.ts (1)
18-20
: Temporary workaround acknowledged.The TODO comment appropriately documents the intent to refactor this via
tanstackViteConfig
. Given the urgency to unblock users, this interim approach is reasonable.Would you like me to open a tracking issue for the planned refactor to
@tanstack/config/vite
?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/router-devtools-core/vite.config.ts
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/router-devtools-core/vite.config.ts
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: |
189b0df
to
7582b93
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/router-devtools-core/vite.config.ts (1)
23-26
: Consider handling non-function external types.Rollup's
external
option accepts string, RegExp, arrays, or functions. IftanstackViteConfig
can ever setexternal
to an array or regex, the early return will break that configuration. However, if this scenario is guaranteed never to occur (as suggested by the comment), the defensive check is acceptable.If you want to handle all external types robustly, apply this diff:
if (typeof config?.build?.rollupOptions?.external !== 'function') { - // this can never happen, but we need to make typescript happy - return + // If external is not a function, keep it as-is but still block solid-js + const staticExternal = config.build.rollupOptions.external + config.build.rollupOptions.external = (id: string) => { + if (id === 'solid-js' || id.startsWith('solid-js/')) return false + // staticExternal could be string, regex, or array - let Rollup handle it + return undefined // Let Rollup apply staticExternal pattern + } + return config }Otherwise, verify with the maintainers that
tanstackViteConfig
always sets a function and this branch is truly unreachable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/router-devtools-core/vite.config.ts
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/router-devtools-core/vite.config.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
package.json
(1 hunks)packages/router-devtools-core/vite.config.ts
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/package.json
📄 CodeRabbit inference engine (AGENTS.md)
Use workspace:* protocol for internal dependencies in package.json files
Files:
package.json
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/router-devtools-core/vite.config.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (1)
package.json (1)
41-41
: @tanstack/config v0.21.0 upgrade is safe
Confirmed support forbundledDeps
, no security advisories, and no breaking changes impacting this usage.
186cfee
to
833bc78
Compare
833bc78
to
19a6ae1
Compare
why do we need to upgrade config? what does this version do that we need? |
@schiller-manuel , I can see that the latest tanstack config has this change: |
The issues that show up in the CI here are indeed unrelated to the PR. The errors show when bumping tanstack/config from 0.16.1 to 0.17.0 or higher on the main branch. @lachlancollins , I can see 0.17.1 move to TypeScirpt, but it appear much stricter already from 0.17.0 - do you know how that can be? |
closes #5420
I'm not sure why upgrading
@tanstack/config
causes the build to fail. I could use some help with that as it seems a bit unrelated. I believe this fix should work once we get past thatSummary by CodeRabbit