Skip to content
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

Hydration fix #523

Merged
merged 20 commits into from
Feb 3, 2023
Merged

Hydration fix #523

merged 20 commits into from
Feb 3, 2023

Conversation

kirill-konshin
Copy link
Owner

@kirill-konshin kirill-konshin commented Jan 18, 2023

Refactored wrapper.useWrappedStore() to just return store with no hydration. All hydration was moved into wrapper.useHydration(props).

Props logic was simplified.

Memo hydration is now called only once per page load, to accommodate server side rendered HTML. All subsequent hydrations are dispatched by layout effect. This means all selectors must take into account the fact that data may be unavailable.

This will be released as 9.x major release.

@voinik pls have a look.

Fixes #522
Fixes #520

Honestly, I find it really weird that old component is not unmounted when new is mounting. Sounds like a bug.

@kirill-konshin
Copy link
Owner Author

kirill-konshin commented Jan 18, 2023

Btw, I also added a HOC for legacy pages wrapper.withHydration(Page), which lead me to idea, what if this HOC will take optional loading component and prevent showing of the page, that is not ready yet:

function Page () {
  const foo = useSelector(...);
  return <div>{foo}</div>;
}

function Loading () { return <div>Loading</div>; }

export default wrapper.withHydration(Page, { Loading });

In this case hook-based wrapper.useHydration is no longer needed.

This smells like old school HOC approach, but abstracts implementation nuances from users.

Alternative API may look like this:

function Page () {
  const { loadingSelector, loading } = wrapper.useHydration();
  const foo = useSelector(loadingSelector(someRealSelector, 'someDefaultValueMaybe?'));
  if (loading) return <div>Loading</div>;
  return <div>{foo}</div>;
}

@daniil4 @voinik @Pikachews @bjoluc thoughts?

@kirill-konshin
Copy link
Owner Author

Implemented the above. Updated readme can be found here: https://github.com/kirill-konshin/next-redux-wrapper/tree/hook-api

I significantly simplified everything. Seems to be the right direction.

README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@bjoluc bjoluc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the right step to me and I love the new simplicity – great job! I'm only a little worried about server-side hydration behavior: Will the server-rendered markup contain anything that's introduced with the HYDRATE action (such as state from cookies in next-redux-cookie-wrapper)?

packages/wrapper/src/index.tsx Outdated Show resolved Hide resolved
@kirill-konshin
Copy link
Owner Author

Few more considerations regarding useHydration vs withHydration:

  1. Hook is more modern, but requires fixing of selectors (state => state?.foo?.bar), which is simple, but consumers will have to do it as part of migration
  2. HOC is older style, and most of libraries these days are abandoning it, but HOC is a bulletproof solution to prevent render of incomplete selectors
  3. BUT in general selectors should always be safe because:
    a. there's no guarantee when portions of state will arrive, and
    b. same selectors may be used on the particular page which dispatches hydration and elsewhere in the app, and HOC will not protect against this

So I am considering to just return loading status without loadingSelector, as this would enforce better coding practices.

Honestly, I have a question to authors of useSelector, because it can't handle useSelector(null), unlike, say, SWR, where useSWR(condition ? '/api/foo' : null) is a perfectly legit pattern.

@bjoluc
Copy link
Contributor

bjoluc commented Jan 19, 2023

So I am considering to just return loading status without loadingSelector, as this would enforce better coding practices.

Yup, fully on board with that. Folks should either use optional chaining in a selector, or let the store ensure the selected path is available at any time. Let's keep things simple on our end; the Readme is bloated enough :D

@kirill-konshin
Copy link
Owner Author

So I am considering to just return loading status without loadingSelector, as this would enforce better coding practices.

Yup, fully on board with that. Folks should either use optional chaining in a selector, or let the store ensure the selected path is available at any time. Let's keep things simple on our end; the Readme is bloated enough :D

Readme have been shortened a bit, and I will cut it down even more ;) it's a direct consequence of tricky Next.js props management. And it will be even worse if I will find a way to make wrapper to work with Server Components and new app folder.

@kirill-konshin
Copy link
Owner Author

I just had a breakthrough moment. I have ditched HYDRATE action, and started to just replay all the actions that happened on server. No more issues with tricky hydration, no more server/client state separation problems. I have deleted a few sections of readme, it's now much simpler.

@bjoluc
Copy link
Contributor

bjoluc commented Jan 23, 2023

I just had a breakthrough moment. I have ditched HYDRATE action, and started to just replay all the actions that happened on server. No more issues with tricky hydration, no more server/client state separation problems. I have deleted a few sections of readme, it's now much simpler.

Congratulations! 🎉 The middleware approach looks really good! I wonder how we didn't consider replaying actions before 😅 Of course, this means actions and reducers must be designed s.t. dispatching the same action multiple times does not alter state (i.e., no toggle actions and friends). But that seems like a fair tradeoff for not needing to maintain HYDRATE reducers! In this regard, I wonder if we should take some

ignoreActions: (action: AnyAction) => boolean

config option in case some actions are not safe/meant to be replayed?

Copy link
Contributor

@bjoluc bjoluc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very good. I'm in favor of renaming the page props from initialStateABC to reduxActionsABC since it's no initial state but actions. And I'd like to have redux in the name to somehow make clear where the props come from / where they belong to. WDYT?

packages/wrapper/src/index.tsx Outdated Show resolved Hide resolved
packages/wrapper/src/index.tsx Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
packages/wrapper/src/index.tsx Outdated Show resolved Hide resolved
@bjoluc
Copy link
Contributor

bjoluc commented Jan 23, 2023

In this regard, I wonder if we should take some

ignoreActions: (action: AnyAction) => boolean

config option in case some actions are not safe/meant to be replayed?

Don't mind my words, I hadn't reviewed index.tsx yet, and hadn't discovered actionFilter 🤦

kirill-konshin and others added 2 commits January 24, 2023 11:41
Co-authored-by: bjoluc <mail@bjoluc.de>
Co-authored-by: bjoluc <mail@bjoluc.de>
@kirill-konshin
Copy link
Owner Author

Looks very good. I'm in favor of renaming the page props from initialStateABC to reduxActionsABC since it's no initial state but actions.

Done.

I wonder how we didn't consider replaying actions before 😅

Better late than never... HYDRATE was also fine. I started to brainstorm how to implement wrapper for new app directory, and got an idea to push updates in a granular way, because there any server component can push bits of new state. But turned out Redux can't be used on server, at least for now. Nevertheless, the idea was quite useful.

@DamienDeloubes
Copy link

Any updates on this 😎. Just experienced this issue also.

@bjoluc
Copy link
Contributor

bjoluc commented Feb 2, 2023

Dear @DamienDeloubes,

as you can see from the activities listed in this PR, @kirill-konshin is actively working on this. The fix became a major rewrite and releasing it with confidence is a time-consuming job, given that @kirill-konshin is mostly left alone working on this library, despite its popularity. So please be patient and consider reading this post on issue bumping.

Thanks!

@kirill-konshin
Copy link
Owner Author

Latest update — I have finally fixed all tests, they are all passing and it all seems to be working fine.

I plan to release it to master, to let people test it in real apps.

You guys are welcome to provide more testing, if you can.

@bjoluc
Copy link
Contributor

bjoluc commented Feb 2, 2023

Great news! I'll likely find some time to plug this into an existing app tomorrow. If you'd like me to, I can also work on improving test coverage then. Re master, let me refer to #194 (comment) – if this becomes master (btw, time to rename to main?), the readme should at least have a notice with a link to the v8 readme at a prominent position...

@kirill-konshin
Copy link
Owner Author

Great news! I'll likely find some time to plug this into an existing app tomorrow. If you'd like me to, I can also work on improving test coverage then. Re master, let me refer to #194 (comment) – if this becomes master (btw, time to rename to main?), the readme should at least have a notice with a link to the v8 readme at a prominent position...

It would be great if you can test it out.

I usually just switch default branch, so that when you open the repo, v7 or v8 is shown.

I have just fixed the coverage too, btw.

@kirill-konshin kirill-konshin merged commit 29fa507 into master Feb 3, 2023
@kirill-konshin
Copy link
Owner Author

V8 is now default branch, this PR has been committed to master.

@kirill-konshin kirill-konshin deleted the hook-api branch February 3, 2023 23:46
@sebastiangrebe
Copy link

@kirill-konshin can we get a prerelease for the v9? I am struggling testing it out on my local with master

@kirill-konshin
Copy link
Owner Author

kirill-konshin commented Feb 7, 2023

@kirill-konshin can we get a prerelease for the v9? I am struggling testing it out on my local with master

https://github.com/kirill-konshin/next-redux-wrapper/releases/tag/9.0.0-rc.1
https://www.npmjs.com/package/next-redux-wrapper/v/9.0.0-rc.1

@fendermany
Copy link

fendermany commented Feb 25, 2023

Hi Kirill,
Thank you for your efforts. I installed the latest version 9, but I don't understand how to use it with getStaticPaths

Code below gives me error useMemo of undefined

Screenshot 2023-02-25 111813

@kirill-konshin
Copy link
Owner Author

You can't dispatch from getStaticPaths. It is used only to create a number of paths, which then are processed by getStaticProps, one by one.

@magicDvlp
Copy link

You can't dispatch from getStaticPaths. It is used only to create a number of paths, which then are processed by getStaticProps, one by one.

It seems the question is about using rtk query and next-redux-wrapper. This is the recommended way to get data inside getStaticProps. Also without the HYDRATE action, the recommended way to use rtk query with the next-redux-wrapper is no longer possible.
Can you give a couple of tips on how to use rtk qveri with next-redux-wrapper now.

@kirill-konshin
Copy link
Owner Author

Why not possible? All actions are recorded and re-dispatched on client. https://github.com/kirill-konshin/next-redux-wrapper/tree/master/packages/demo-redux-toolkit use the example. Just make sure to dispatch all actions from getStaticProps, and wrap it properly. getStaticPaths can remain unwrapped, you only need it to get the paths. This is how Next.js is architected.

@magicDvlp
Copy link

But if I use rtk query to get the number of paths in getStaticPaths, I need to dispatch an action to call the endpoint. As shown in the screenshot.
And do I understand correctly that this way of using rtl query is no longer supported?

@fendermany
Copy link

fendermany commented Mar 3, 2023

I tried getting all patches in getStaticPatch with fetch, but finally, I got 404 when accessing a mapped page. And of course, you can't use getStaticProps without getStaticPaths on [param] pages. It looks like we need to check that the paths are created before doing anything else. Any ideas?

@Distortedlogic
Copy link

any update on when there will be a new release with this fix?

@its-monotype
Copy link

its-monotype commented Nov 1, 2023

Any updates on when it will be released?

@doskovicmilos
Copy link

doskovicmilos commented Nov 21, 2023

The solution with wrapper.useHydration(props); will not perform rendering on SSR in Layout if getLayout is used for Per-Page Layouts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
9 participants