[Bug] resourcesRoutes list is sensitive to ordering #2910

kopax · 2019-02-21T14:50:55Z

We have the following resourcesRoutes:

    <Resource
      name="project-management/projects"
      list={ProjectList}
      edit={ProjectEdit}
      create={ProjectCreate}
      show={ProjectShow}
    />,
    <Resource
      name="project-management/projects/services"
      list={ProjectServiceList}
      edit={ProjectServiceEdit}
      create={ProjectServiceCreate}
      show={ProjectServiceShow}
    />,

This fail to generate a working <Route />, because project-management/projects/services will catch before project-management/projects.

Instead we must order it DESC :

    <Resource
      name="project-management/projects/services"
      list={ProjectServiceList}
      edit={ProjectServiceEdit}
      create={ProjectServiceCreate}
      show={ProjectServiceShow}
    />,
    <Resource
      name="project-management/projects"
      list={ProjectList}
      edit={ProjectEdit}
      create={ProjectCreate}
      show={ProjectShow}
    />,

This will make a valid <Route />.

It can also be solved using exact props, and customRoutes, but this is too sensitive for being an expected behaviour, and it is not specified anywhere in the documentation.

If you agree, this ticket consists of taking all the user props from <Resource /> and pass them to the <Route /> component.

https://github.com/marmelab/react-admin/blob/master/packages/ra-core/src/Resource.tsx#L80

The text was updated successfully, but these errors were encountered:

fzaninotto · 2019-02-21T14:58:48Z

This fail to generate a working <Route />, because project-management/projects/services will catch before project-management/projects/visibilities.

I don't see how that can be possible. On which URL?

Taking all the user props from <Resource /> and pass them to the <Route /> component.

Which props are you thinking about? Also, one Resource creates up to 4 routes, I'm not sure that additional props make sense for all 4 routes.

I suspect you have a very custom use case, in which case I advise implementing your own <Resource> component.

kopax · 2019-02-21T15:16:15Z

I don't see how that can be possible. On which URL?

The router tries to match your routes in the order they are defined, this is how react-router work.

remix-run/react-router#1677 (comment)

Which props are you thinking about?

exact
- https://github.com/ReactTraining/react-router/blob/master/packages/react-router/docs/api/Route.md#exact-bool
- https://stackoverflow.com/questions/49162311/react-difference-between-route-exact-path-and-route-path

If we allow exact, why not allowing also:

I'm not sure that additional props make sense for all 4 routes.

I think it should:

/ maps to the list component
/create maps to the create component
/:id maps to the edit component
/:id/show maps to the show component

All of these path can be extended, for instance, these are valid and may cause conflict on ordering:

/further
/create/further
/:id/further
/:id/show/further

The /further routes must be declared before the Resource one. This common scenario can happen when using customRoutes but also happen within resourceRoutes.

I suspect you have a very custom use case, in which case I advise implementing your own <Resource> component.

Unfortunately, this is the most simple case we can have. People must order their route by priority.

Another possibility to solve this would be to add the exact props by default to all <Resource />.

fzaninotto · 2019-02-21T15:19:10Z

The router tries to match your routes in the order they are defined, this is how react-router work.

I don't understand how an URL like project-management/projects/visibilities can be caught by a route like project-management/projects/services. The two don't match.

kopax · 2019-02-21T15:22:24Z

You are right, I was pasting the wrong code part. I just updated it with the right one.

I have prepared examples to demonstrate the issue and possible fix:

What we have and what fail: https://codesandbox.io/s/m94z27loo9

<Route path="/roster" component={Roster} />
<Route path="/roster/schedule" component={Schedule} />

Result: /roster declared first will catch /roster/schedule

What we must do: https://codesandbox.io/s/q7q25y76j9

<Route path="/roster/schedule" component={Schedule} />
<Route path="/roster" component={Roster} />

Result: /roster declared second will not catch /roster/schedule

What we could do by allowing props such as exact on <Resource />: https://codesandbox.io/s/qlmmozyoq6

<Route path="/roster" component={Roster} exact />
<Route path="/roster/schedule" component={Schedule} />

Result: /roster expect to be exact second will catch /roster/schedule

Adding exact by default to all <Resource />

This is also a valid action and should be considered.

Currently (2) is the one action people can do but... setting order on a high amout of <Resource /> can be a pain and error prone.

In our case, (2) will certainly bring route conflict issues because we group resourcesRoutes from packages within node_modules. It will not always be possible to set the order.

Without exact, on our side, we can:

Declare <Route /> instead of <Resource />.
Using a custom <Resource />.
Hook a fixed Resource.js using webpack.resolve.alias and append exact like this.
Sort <Resource /> by props.name.length DESC: (resourcesRouteList.sort((a, b) => a.props.name.length < b.props.name.length ? 1 : -1)).

(3) and (4) together are imoo the prefered action as they give the more flexibility and the best default configuration. We fixed with sorting.

fzaninotto · 2019-02-21T16:22:38Z

I'm wondering why we don't use exact in the first place. It should not break BC on existing apps, am I right @djhi?

fzaninotto · 2019-02-21T16:24:02Z

@kopax thanks for the detailed description. There is indeed a bug.

kopax · 2019-02-21T16:24:44Z

Thanks do you want me to fix this? What action do you prefer?

fzaninotto · 2019-02-21T16:25:35Z

Let's wait for @djhi's feedback before deciding on a fix.

kopax · 2019-03-01T09:41:20Z

@djhi any though about this one?

djhi · 2019-03-19T06:30:04Z

Sorry, missed this one. I'm ok for using exact ou our routes.

kopax · 2019-03-19T19:00:11Z

I was hitting this more and more while adding resources. Can we add this to next release? Thanks.

kopax · 2019-03-22T07:09:32Z

@fzaninotto @djhi, I have found that MuiTab does not work well with exact: true, because the <Switch /> in charge of rendering them will not trigger the rendering when exact: true is set on a parent <Route /> in a parent <Switch />.

I suggest the following implementation:

We allow exact props on Resource|List|Create|Show|Edit on <Resource /> creation. (ex: <Resource exact={false} />)
We set exact: true by default.
User who wants MuiTab must set exact to their resource route (BREAKING CHANGE if (2) is true)
Using (1) <Resource list={(props) => <List {...props} exact />} />, during crud <Route /> creation, we omit props.exact on the <List|Crete|Edit|Show /> and use their props.exact on their respective <Route />.

Regarding (4), because they are all tightly coupled to the URL, it is not dirty to apply a route props on them. This would only work on <Resource /> declaration and we can keep using <Show|Create|Edit /> on customRoutes as usual (but not <List /> yet... #2903).

If you do not want a BREAKING CHANGE, we can skip (2) and keep exact: false by default.

Doing this will have more benefits than simply adding exact: true:

User decide their route logic 100%
Tabs can work, and there is no bug anymore except this one:
- /:id/show/2 => show tab 2
- /:id/show/2/something/awesome => If <Link to={to}> exists to this path and the page is missing, it should display 404 view, instead it will display /:id/show/2 (due to the absence of exact: true for that route).

The issue is specific to react-router and it is not a bug, it's the configuration part. We can't do much about it, a warning should be added to the documentation when using TabbedForm.

If you agree with or without (2), I can implement it.

santaclauze · 2019-03-25T04:21:39Z

Hey, I have a similar issue.

My resource route:

    <Resource
      name={"user-management/users"}
      list={UserList}
      edit={UserEdit}
      create={UserCreate}
      show={UserShow}
    />

My custom route:

    <Route path={"/:resource/:id/authorities"} component={AuthoritiesEdit} />

The route couldn't render. We use the sort fix, but because the sort happen on two separate list, it is still not sufficient to do it userland.

The quickest fix i found was the following:

-  <Route path={"/:resource/:id/authorities"} component={AuthoritiesEdit} />

+  <Route path={"/authorities/:resource/:id"} component={AuthoritiesEdit} />

Could we please support exact on resource instead of forbidding path?

kopax · 2019-04-02T09:36:08Z

We need to give back the control to the end user, could we please have an update on this issue? I'll gladly help to implement the next specification to see it release soon.

As a reminder, tabs needs exact: false while we generally want to have exact: true for our route, but that's not always true if we encapsulate <Switch /> within each over.

fzaninotto · 2019-12-03T16:27:34Z

I'm not fond of any of the solutions you've suggested:

Making TabbedForm fail by default is a no-go. It is used more often than resources with overlapping names
The alternative, defaulting to exact=false, which is what we do, means that people will get the bug you described. Whatever the solution is, they'll have to search in the documentation first.
It's not possible to have exact=true on CRUD routes AND TabbedForm, and that's a serious limitation.

Also, using resource names that are actually paths is mixing responsibilities between the frontend and the backend. In react-admin's design, it's the data provider's responsibility to translate a resource name into an API URL. This makes me think that the problem you're having in the first place it due to a wrong usage of react-admin.

So I maintain my initial reaction: your use case isn't supported by react-admin, and you should write your own Resource component.

I'll close this issue and the related PR. Thank you anyway for helping us understand the problem.

kopax · 2019-12-03T20:33:16Z

Also, using resource names that are actually paths is mixing responsibilities between the frontend and the backend. In react-admin's design, it's the data provider's responsibility to translate a resource name into an API URL. This makes me think that the problem you're having in the first place it due to a wrong usage of react-admin.

That could definitely be done in the dataProvider.

If we want to have similar links between the two, 99% of the code is working. react-router give use the grain to configure route, tabbed route, anything. The current interface of Resource doesn't give us the grain to configure the route entirely, and this limitation comes from react-admin.

I offered one more solution that you have not listed. It is quite good and it's the one we have chosen. Using sorting (by PATH desc) won't have any effect on the current strategy for all use cases. I recommend using that one.

Sort <Resource /> by props.name.length DESC: (resourcesRouteList.sort((a, b) => a.props.name.length < b.props.name.length ? 1 : -1)).

Since this is not blocking us as this can be solved userland with an ugly hack, it's not dramatic if it's not integrated but that should be also considered as a final choice in your listing.

fzaninotto added the needs more info label Feb 21, 2019

kopax changed the title ~~[Bug] resourceRoutes list is sensitive to ordering~~ [Bug] resourcesRoutes list is sensitive to ordering Feb 21, 2019

fzaninotto added bug and removed needs more info labels Feb 21, 2019

kopax mentioned this issue Mar 18, 2019

fix(Resource): Added exact to all <Route /> #3021

Closed

kopax mentioned this issue Mar 20, 2019

fix(resourcesRoutes): Added prop exact to all resources routes #3026

Closed

fzaninotto assigned kopax Mar 21, 2019

fzaninotto closed this as completed Dec 3, 2019

kopax mentioned this issue Jun 9, 2020

Does <Resource /> support inner routes ? #4921

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Bug] resourcesRoutes list is sensitive to ordering #2910

[Bug] resourcesRoutes list is sensitive to ordering #2910

kopax commented Feb 21, 2019 •

edited

Loading

fzaninotto commented Feb 21, 2019

kopax commented Feb 21, 2019 •

edited

Loading

fzaninotto commented Feb 21, 2019

kopax commented Feb 21, 2019 •

edited

Loading

fzaninotto commented Feb 21, 2019

fzaninotto commented Feb 21, 2019

kopax commented Feb 21, 2019

fzaninotto commented Feb 21, 2019

kopax commented Mar 1, 2019

djhi commented Mar 19, 2019

kopax commented Mar 19, 2019

kopax commented Mar 22, 2019 •

edited

Loading

santaclauze commented Mar 25, 2019 •

edited

Loading

kopax commented Apr 2, 2019

fzaninotto commented Dec 3, 2019

kopax commented Dec 3, 2019 •

edited

Loading

[Bug] resourcesRoutes list is sensitive to ordering #2910

[Bug] resourcesRoutes list is sensitive to ordering #2910

Comments

kopax commented Feb 21, 2019 • edited Loading

fzaninotto commented Feb 21, 2019

kopax commented Feb 21, 2019 • edited Loading

fzaninotto commented Feb 21, 2019

kopax commented Feb 21, 2019 • edited Loading

fzaninotto commented Feb 21, 2019

fzaninotto commented Feb 21, 2019

kopax commented Feb 21, 2019

fzaninotto commented Feb 21, 2019

kopax commented Mar 1, 2019

djhi commented Mar 19, 2019

kopax commented Mar 19, 2019

kopax commented Mar 22, 2019 • edited Loading

santaclauze commented Mar 25, 2019 • edited Loading

kopax commented Apr 2, 2019

fzaninotto commented Dec 3, 2019

kopax commented Dec 3, 2019 • edited Loading

kopax commented Feb 21, 2019 •

edited

Loading

kopax commented Feb 21, 2019 •

edited

Loading

kopax commented Feb 21, 2019 •

edited

Loading

kopax commented Mar 22, 2019 •

edited

Loading

santaclauze commented Mar 25, 2019 •

edited

Loading

kopax commented Dec 3, 2019 •

edited

Loading