web icon indicating copy to clipboard operation
web copied to clipboard

feat: implement `useSsrState` hook

Open xobotyi opened this issue 2 years ago • 21 comments
trafficstars

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?

xobotyi avatar Nov 21 '22 18:11 xobotyi

Codecov Report

Merging #1023 (b237521) into master (1c1d098) will increase coverage by 0.00%. The diff coverage is 100.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

codecov[bot] avatar Nov 21 '22 18:11 codecov[bot]

I will have time to review this on Friday.

I have two questions about this:

  1. Will there be an option to use the hooks as usual with the initializeWithValue prop? 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 the initializeWithValue option.
  2. 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?

ArttuOll avatar Nov 23 '22 16:11 ArttuOll

  1. Id rather remove all options. This hook can be reworked to use variables instead of context, to avoid context bloating.
  2. Yes the plan is to rework all currently isomorphic hooks and make major release.

xobotyi avatar Nov 24 '22 15:11 xobotyi

But there will be an issue with typing, that I have no idea how to overcome

xobotyi avatar Nov 24 '22 15:11 xobotyi

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)

xobotyi avatar Nov 28 '22 08:11 xobotyi

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.

kylemh avatar Dec 07 '22 14:12 kylemh

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.

xobotyi avatar Dec 22 '22 11:12 xobotyi

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.

xobotyi avatar Jan 02 '23 09:01 xobotyi

does something like this help? https://twitter.com/benlesh/status/1610298056540397568?s=46&t=Ar1ixjc9BhyFJS15ooM1Fg

kylemh avatar Jan 04 '23 01:01 kylemh

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

xobotyi avatar Jan 04 '23 11:01 xobotyi

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.

kylemh avatar Jan 04 '23 15:01 kylemh

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

xobotyi avatar Mar 27 '23 23:03 xobotyi

That still only adjusts the types, no? Won't all client-only users suffer from an extra render and bundled code?

kylemh avatar Mar 28 '23 12:03 kylemh

Thre is basically no difference, afaik, since i don believe current useStorageValue has SSR-compliant code eliminated

xobotyi avatar Mar 28 '23 14:03 xobotyi

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.

kylemh avatar Mar 28 '23 15:03 kylemh

This pr brings such context value to switch all hooks it one move

xobotyi avatar Mar 28 '23 15:03 xobotyi

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.

kylemh avatar Mar 28 '23 16:03 kylemh

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?

ArttuOll avatar Apr 03 '23 16:04 ArttuOll

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?

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?

ArttuOll avatar Apr 05 '23 11:04 ArttuOll

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.

xobotyi avatar Apr 05 '23 12:04 xobotyi

@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🙈

xobotyi avatar May 14 '23 10:05 xobotyi