danielr18 / connected-next-router

A Redux binding for Next.js Router compatible with Next.js.
MIT License
109 stars 30 forks source link

Feature request: createMatchSelector to replace connected-react-router #59

Open Guneetgstar opened 3 years ago

Guneetgstar commented 3 years ago

I am migrating from client side rendering to next.js for SSR. Although connected-next-router is supposed to replace connected-react-router (with the basic analogical name) it does not include a very basic functionality to match to opponent. I haven't covered everything thats missing but found that this can be included in the library itself very easily to fast line the migration process.

danielr18 commented 3 years ago

@Guneetgstar I think it's a good idea to add the same selectors connected-react-router has, would you be interested in opening a PR?

Guneetgstar commented 3 years ago

@danielr18 Yeah I think I can do it. But currently I am stuck at how to change redux state using next/link API. Do you have a workaround?

Guneetgstar commented 3 years ago

Oh, I fixed it, that was a tiny configuration mistake. There was a wrong router reducer set.

Guneetgstar commented 3 years ago

Hi @danielr18 I looked further with the API and found that this repo does not give a porting option for connected-react-router users to next project as it not only lacks features that I just mentioned above (selector functions) but also doesn't have the same API for the very basic CallHistoryMethodAction like push and replace. Ex: This would break in connected-next-router:-

 replace({pathname: `${paths.PRODUCTS}/${product.category}`, search: `?id=${id}`})

Here you mentioned the react-router but it supports this sort of URL de-structuring as well.

In fact the whole implementation is limited to similar behaviour only and a developer just cant include connected-next-router and remove connected-react-router where ever used as a migration script as it would involve a lot of breaking changes after that. To make it more useful we should provide an easy migration guide or at least include the caveats in the README.md

danielr18 commented 3 years ago

You are right, it would be good to document those differences in a migration guide. The good this is Next.js 10 has automatic href resolving, so the as param wouldn't need to be added while migrating dynamic routes to pages.

Guneetgstar commented 3 years ago

Thanks for the mention about nextjsv10 feature I can now the above example works like:

replace(
               `${paths.PRODUCTS}/[[...slug]]/`,
               `${paths.PRODUCTS}/${product.category}?${new URLSearchParams({id:id})}`
 )

But I still see that replace function only works with string parameters not Url or UrlObject and I think I should open a new issue for it.

Guneetgstar commented 3 years ago

Hi @danielr18 again, although I really wanted to contribute, this project is maintained in TypeScript and I know very less about it and I guess it would be a easy task for you to make a PR for what we have discussed.

Guneetgstar commented 3 years ago

Please check the PR.