-
-
Notifications
You must be signed in to change notification settings - Fork 753
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
fix: router stream injection #3216
Conversation
…ection # Conflicts: # packages/react-router/src/ScriptOnce.tsx
View your CI Pipeline Execution ↗ for commit 91bf350.
☁️ Nx Cloud last updated this comment at |
More templates
@tanstack/arktype-adapter
@tanstack/create-router
@tanstack/directive-functions-plugin
@tanstack/eslint-plugin-router
@tanstack/create-start
@tanstack/history
@tanstack/react-cross-context
@tanstack/react-router
@tanstack/react-router-with-query
@tanstack/router-cli
@tanstack/router-devtools
@tanstack/router-generator
@tanstack/router-plugin
@tanstack/router-vite-plugin
@tanstack/server-functions-plugin
@tanstack/start
@tanstack/start-api-routes
@tanstack/start-client
@tanstack/start-config
@tanstack/start-plugin
@tanstack/start-router-manifest
@tanstack/start-server
@tanstack/start-server-functions-client
@tanstack/start-server-functions-fetcher
@tanstack/start-server-functions-handler
@tanstack/start-server-functions-server
@tanstack/start-server-functions-ssr
@tanstack/valibot-adapter
@tanstack/virtual-file-routes
@tanstack/zod-adapter
commit: |
90abc8e
to
1163814
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.
The removal of a user-configurable serializer/transformer is quite a breaking change for Start.
Should this go forward "as-is", we'll need to update it on the breaking changes tracking list (#2863).
* @link [API Docs](https://tanstack.com/router/latest/docs/framework/react/api/router/RouterOptionsType#transformer-property) | ||
* @link [Guide](https://tanstack.com/router/latest/docs/framework/react/guide/ssr#data-transformers) | ||
*/ | ||
transformer?: RouterTransformer |
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.
At this current stage, the removal of the user-configurable transformer
property would be a breaking change.
If the argument is that since this has the most impact on Start and that it's fine to make these breaking changes, could we perhaps move these Start-specific options under their own key?
Something like this perhaps?
createRouter({
// ...
server: { // or framework / start
serializer: {}
}
})
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.
I think we need a different API approach to configure server stuff. Ideally, it does not touch the router at all.
|
||
```tsx | ||
import { SuperJSON } from 'superjson' | ||
|
||
const router = createRouter({ | ||
transformer: SuperJSON, | ||
serializer: SuperJSON, |
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.
As far as I can see, at this current time, this action of setting serializer: SuperJSON
is not possible since the serializer
property (previously named transformer
) no longer exists on RouterOptions
.
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.
the whole non-start SSR story needs a major overhaul. let's do this separately
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.
👍🏼
Are we committing to removing user-configurable transformer/serializer
though?
If not, I think it'd make sense to introduce a bug where we leave the RouterOptions
interface "as-is" but disable functionality in code. Then wire it back up in another PR with any renaming that needs to be done.
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.
for now it cant be configured. but this functionality should be re-added at some point.
f22015e
to
91bf350
Compare
the |
No description provided.