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

TabbedForm w/ syncWithLocation shows nothing if resource id contains URL-encoded chars #8380

Closed
stesie opened this issue Nov 10, 2022 · 4 comments
Labels

Comments

@stesie
Copy link
Contributor

stesie commented Nov 10, 2022

What you were expecting:

TabbedForm renders one visible tab.

What happened instead:

No tab is visible.

Steps to reproduce:

Use TabbedForm on a resource with string-type ids + have an id that contains a URL-encoded char, e.g. "space" which is encoded as "%20"

Related code:

https://github.com/marmelab/react-admin/blob/master/packages/ra-ui-materialui/src/form/TabbedFormView.tsx#L85

  • resolvedPath.pathname is like /posts/better no spaces here
  • location.pathname is like /posts/better%20no%20spaces%20here

... hence no match. For no route. And therefore all tabs hidden.

StackBlitz

https://stackblitz.com/edit/github-c52e17

click the (first) Post named "Post to break stuff"

Other information:

Environment

  • React-admin version: 4.5.1
  • Last version that did not exhibit the issue (if applicable):
  • React version: 17
  • Browser:
  • Stack trace (in case of a JS error):
@slax57 slax57 added the bug label Nov 14, 2022
@slax57
Copy link
Contributor

slax57 commented Nov 14, 2022

Reproduced, thanks!
Would you like to open a PR to fix this?

@stesie
Copy link
Contributor Author

stesie commented Nov 15, 2022

I had another look at this today. I don't think this is an issue with react-admin, but with react-router-dom instead.
Interestingly the above works just fine with react-router-dom <= 6.4.2 and stopped working with (current) version 6.4.3

There's also (other, yet similar) issues reported upstream: remix-run/react-router#9580

@stesie
Copy link
Contributor Author

stesie commented Jan 17, 2023

@slax57 after react-router upstream fixed the issue I've had another look at this issue and noticed that react-admin unfortunately is still broken. Things seem to go disfunctional when using AdminRouter router implementation, as provided by react-admin.

I've created two more stackblitz sandboxes. A very basic one that just uses plain AdminRouter, outputting both location.pathname and resolvedPath.pathname.
See https://stackblitz.com/edit/react-ts-dw3ein and follow the link to an URL hash with a space char in it.

It should output:

Hello World :)
location.pathname: /bla/foo%20bar
resolvedPath.pathname: /bla/foo bar

i.e. mismatching URLs.

AdminRouter which effectively just renders react-admin's HistoryRouter contains this comment:

A router that accepts a custom history.
To remove once remix-run/react-router#7586 is merged.

... interestingly the PR is merged for quite a while.

Therefore I created another sandbox trying it out with react-router's HistoryRouter.
See here: https://stackblitz.com/edit/react-ts-g75evm

... which outputs matching pathnames, i.e. it works.

Please note, that I'm using createHashHistory from @remix-run/router
... opposed to AdminRouter which imports it from history package.

Or to put it differently: the types of history object are different. Hence it would possibly be considered a breaking change, just swapping them. But it also feels wrong to fix react-admin's HistoryRouter implementation.

Also there's a deprecated hint to the history prop.

Please let me know in which direction you tend to go.

PS: To me this is not ultra important, I've justed disabled syncwithLocation and am fine with it. I just had a look since I've opened this issue and was wondering if there's still a problem...

@fzaninotto
Copy link
Member

Thanks for your feedback.

This is a rare use case, it's fixable in user land with disableSyncWithLocation, and we can't properly fix it for all versions of react-router. So I'll close this bug as "won"t fix".

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

No branches or pull requests

3 participants