-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix(solid-router): prevent client effects running on server #4621
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
View your CI Pipeline Execution ↗ for commit ec76f6a
☁️ Nx Cloud last updated this comment at |
More templates
@tanstack/arktype-adapter
@tanstack/directive-functions-plugin
@tanstack/eslint-plugin-router
@tanstack/history
@tanstack/react-router
@tanstack/react-router-devtools
@tanstack/react-router-with-query
@tanstack/react-start
@tanstack/react-start-client
@tanstack/react-start-plugin
@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-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-plugin
@tanstack/solid-start-server
@tanstack/start-client-core
@tanstack/start-plugin-core
@tanstack/start-server-core
@tanstack/start-server-functions-client
@tanstack/start-server-functions-fetcher
@tanstack/start-server-functions-server
@tanstack/valibot-adapter
@tanstack/virtual-file-routes
@tanstack/zod-adapter
commit: |
The SSR test passed on CI so overall this approach seems to be working at least. I did try Vitest's test projects feature to combine client and server tests into same config, but I could not get it working, could very well be that it isn't the right tool for the job. Giving up on trying to improve it any further for now. Currently implemented approach is borrowed from solid-primitives. As an added note to the main item of this pull request, Transitioner largely seems to be consisting of client only behaviors and one might as well just short circuit it at some specific point with |
Im honestly not sure about this PR. Is there any specific issue this solves or just trying to run less on the server when its not needed? |
packages/solid-router/src/utils.ts
Outdated
|
||
export const useLayoutEffect = | ||
typeof window !== 'undefined' ? Solid.createRenderEffect : Solid.createEffect | ||
export const useLayoutEffect = isServer |
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'm not sure I like this indirection but i guess it was kinda there already
The latter. Just rubs me the wrong way that there are effects queued and then ran after server has already rendered the output. At closer inspection I'd say that the entire Transitioner is pretty much unnecessary on server side, the code that's before the client effects is just setup for the effects. So we could just introduce |
we should keep react and solid impl aligned when it comes to Transitioner. if there are changes applicable for both then please go ahead |
1058180
to
d6f745d
Compare
I reworked the commits such that both React and Solid Transistioners now bail out as early out as possible on the server. I may have misjudged the usefulness of Solid's native
For now I do not have any further changes to propose, the change set as it is fixes my concern of Solid doing more work than React and I guess there's a slight win for React as well unless somehow returning early on server is illegal. |
what would be needed to fully support isServer when building the server part? |
Nothing on the library side other than using it, it should be a bundle time optimization. I cannot find my test code for server output anymore, everywhere I look it's just a plain import for server code, so I must have been wrong. All the ones I could find are client code where it does evaluate to false and strip away dead branches. The tests failed due going against hook rules in React 🤡. I am giving up. I can't make this optimization work in both without diverging the implementations, so I guess I'll use |
d6f745d
to
b5c691d
Compare
can we have separate ClientTransitioner and a noop ServerTransitioner impl that we render depending on the env? |
Yeah I suppose that can be done. It's effectively a client-only component, so we can probably avoid creating the no-op if we selectively render it. We could simply check for server at the usage and then not render it. This would be my preferred choice as Transitioner cannot be used externally and there is already some conditional rendering being done at the only usage. {!router.isServer && <Transitioner />} Alternatively we could hide the isServer check inside Transitioner. This would only be an improvement in my opinion if it was part of public API. function Transitioner() {
const router = useRouter()
return router.isServer ? null : <ClientTransitioner />
} Or export a Transitioner impl based on some environment check which probably is the most literal interpretation of your suggestion: export const Transitioner = typeof window !== 'undefined' ? ClientTransitioner : (() => null) Depending on bundler setup this can optimize away the unused branch for a given environment, but it's not entirely clear to me when these optimizations happen and don't happen, so I'd save this variant for a discussion in the future instead. When no optimizations take place, we'd end up shipping both branches to client, which is no better than just opting to do the check during runtime using options 1 or 2. Lastly, I was thinking of just wrapping Transitioner with ClientOnly: <ClientOnly>
<Transitioner />
</ClientOnly> Unit tests seem to pass with this, but compared to the first option, I think this will delay the rendering of Transitioner in client by one render cycle, which might be fine, though I am unsure. Either way the ClientOnly usage will be some overhead and I'm more confident in option 1 working right than this. If it wasn't obvious already, I'd just stick to option 1 for now. |
solution 1 looks good to me. FYI i am adding a static |
b5c691d
to
ec76f6a
Compare
This is exploration of the comment I posted on #4574 (comment).
Using
Solid.createRenderEffect
in SSR will execute the effects after the render takes place. To prevent server from doing meaningless work, we can use theuseLayoutEffect
utility function, which will make the server useSolid.createEffect
instead, preventing such effects from being ran on server. Client side there are no differences asSolid.createRenderEffect
is still used. This is closer to how it works in React version today.To properly test this, we need true SSR with
environment: 'node'
andsolidPlugin({ ssr: true })
in Vite, because:jsdom
environment provideswindow
, which causes wrong branch inuseLayoutEffect
to be chosen. This is fixable by replacingtypeof window !== 'undefined'
check withisServer
fromsolid-js/web
, resulting in a marginally smaller client bundle, too (This was decided against in order to keep the code as similar to react-router).ssr: true
, we do not have access torenderToString
.For now I added the SSR config to the same vite config via
--mode
switch. However I am unhappy with the way how nowpackage.json
script needs to launch vitest twice:"test:unit": "vitest && vitest --mode server"
. Suggestions on better format or alternatives would be most welcome.