web
web copied to clipboard
feat: implement `useSsrState` hook
What new hook does?
Provides stable value that other hooks can use to determine that applocation is in SSR mode.
Sad enough there are no reliable markers that application is hydrated after SSR - therefore we have to introduce own context.
Checklist
- [x] Have you read contribution guideline?
- [x] Does the code have comments in hard-to-understand areas?
- [ ] Is there an existing issue for this PR?
- link issue here
- [x] Have the files been linted and formatted?
- [x] Have the docs been updated?
- [x] Have the tests been added to cover new hook?
- [x] Have you run the tests locally to confirm they pass?
Codecov Report
Merging #1023 (b237521) into master (1c1d098) will increase coverage by
0.00%. The diff coverage is100.00%.
@@ Coverage Diff @@
## master #1023 +/- ##
=======================================
Coverage 99.61% 99.61%
=======================================
Files 61 62 +1
Lines 1041 1048 +7
Branches 163 163
=======================================
+ Hits 1037 1044 +7
Misses 2 2
Partials 2 2
| Impacted Files | Coverage Δ | |
|---|---|---|
| src/index.ts | 100.00% <100.00%> (ø) |
|
| src/useSsrState/useSsrState.tsx | 100.00% <100.00%> (ø) |
:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more
I will have time to review this on Friday.
I have two questions about this:
- Will there be an option to use the hooks as usual with the
initializeWithValueprop? If a user uses only a couple of our hooks in their app, it might be annoying for them to have to wrap their whole application in a context provider and they would probably rather use theinitializeWithValueoption. - Partly related to the previous question: what's the plan after this is merged? Will we refactor all isomorphic hooks to use this under the hood?
- Id rather remove all options. This hook can be reworked to use variables instead of context, to avoid context bloating.
- Yes the plan is to rework all currently isomorphic hooks and make major release.
But there will be an issue with typing, that I have no idea how to overcome
Before merge we have to figure out how to make proper types for isomorphic hooks. As for now - it is pretty easy to alter return types basing of function parameter, but in case of switching to this hook - it is pretty much impossible to type it properly (at leas in my mind, research needed)
This is very tricky. I wanted throw a suggestion out there:
What if we split the react-hookz/web to have client-only and SSR-ready hooks? The type safety would be automatic and developing on the hooks from within this repo may be a bit simpler because the contexts are separated by a folder.
Basically, thinking back to https://github.com/react-hookz/web/issues/1000 there'd be a useMediaQuery exported from @react-hookz/web/client-rendered and from @react-hookz/web/server-rendered. It would also let people who deal with hybrid applications leverage linting to prohibit the wrong folder from being imported within each context.
It will simply double the amount of supported code and will lead to issues when developer uses different hooks in different places. Linter for this case is weak excuse.
What i'm thinking now - we can offer type extension for SSR users, and basing on that extension all types for SSR-sensitive hooks will switch to SSR-compatibility mode. Guess it is possible just dont yet know how.
does something like this help? https://twitter.com/benlesh/status/1610298056540397568?s=46&t=Ar1ixjc9BhyFJS15ooM1Fg
Nah, it is not a problem to set state - problem is in typings of hooks return. Now it works basing off parameters value, with this hook i dont yen know how exactly solve type alternation, therefore this hook is not merged yet
I got hung up on the idea that we could conditionally use a different hook for https://github.com/react-hookz/web/issues/1000 and maybe define the initial value within only useEffect, but that doesn't solve the issue you mentioned earlier of strict mode complaining about different values on subsequent renders.
Ignore me.
Just as an idea: what SSR users think about the solution i18n uses: https://react.i18next.com/latest/typescript
So basically it will require to redeclare some types, along with such hook (that i'm planning to migrate from conext to globally stored variable) -thus youll have a single change to make all compatible hooks to switch to SSR state
That still only adjusts the types, no? Won't all client-only users suffer from an extra render and bundled code?
Thre is basically no difference, afaik, since i don believe current useStorageValue has SSR-compliant code eliminated
hmmm the types don't solve the bigger DX problem which is that depending on your apps context, you'd need to provide arguments to certain hooks 100% of the time.
Maybe we can do like a top-level React-Hookz Provider where we can give ourselves context regarding the users app being client-only or having any server-side code?
I think this would still mean conditional code though in a way you've previously pointed out has problems.
Alternatively, we could go full steam ahead and client-only users will just suffer the inconvenience.
This pr brings such context value to switch all hooks it one move
lol yes it does 🤦🏼 ! sorry i was stuck thinking about that old PR and not this PR.
then yes, i love it in conjunction with the TS idea. seems like the best solution.
It recently occurred to me that the new useSyncExternalStorage hook introduced in React 18 might allow us to remove some of the initializeWithValue parameters in our hooks.
It allows components to subscribe to external stores (in our case, browser APIs) and make React aware of their changes. The hook has a parameter called getServerSnapshot which allows us to define what the hook should return in a server environment and during hydration. To me it sounds like it solves the exact same problem that we are addressing with the initializeWithValue parameter.
Any thoughts?
It recently occurred to me that the new
useSyncExternalStoragehook introduced in React 18 might allow us to remove some of theinitializeWithValueparameters in our hooks.It allows components to subscribe to external stores (in our case, browser APIs) and make React aware of their changes. The hook has a parameter called
getServerSnapshotwhich allows us to define what the hook should return in a server environment and during hydration. To me it sounds like it solves the exact same problem that we are addressing with theinitializeWithValueparameter.Any thoughts?
As an experiment, I attempted to port useWindowSize to use useSyncExternalStore under the hood. It greatly simplified the hooks implementation by removing all effects and the need for a separate initializeWithValue parameter. I see this as having huge potential for other hooks as well.
However, our toolchain would need some upgrading. The React teams has provided a package called use-sync-external-store which could enable users to use the hook in older React versions. When using this package, I couldn't get the server-side tests to work, which might be explained by the fact that react-hooks-testing-library is quite outdated since it has been merged to @testing-library/react and development continued there. However, starting from version 13, @testing-library/react has only supported React 18, which is problematic for us, since we attempt to support React versions >16.8.
Perhaps it would be time to reconsider which React versions we support?
Sadly had a few time lately, therefore havent tried that hook.
I'll try to cut some time this week to have a closer look.
@ArttuOll i just rememebered why we still not using react 18 and its hooks =)
the teting library do not support react 18, the solution they offer is switching to @testing-library/react that does not have any solution for SSR testing - they offer to write own testing library🙈