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

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

Open
Guneetgstar opened this issue Oct 31, 2020 · 8 comments
Labels
good first issue Good for newcomers

Comments

@Guneetgstar
Copy link
Contributor

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 danielr18 added the good first issue Good for newcomers label Oct 31, 2020
@danielr18
Copy link
Owner

@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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

@Guneetgstar
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

Please check the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants