feat(Redirect): customRoutes now accept <Redirect />

[kopax]

Fix #2767

[kopax]

This work fine, this is how we deal with this in our code base:

• Redirect: route.props.from can be left undefined for the general wildcard (404) but it always contains a route.props.to.
• Route: it always contains a route.props.path

To deal with <Route /> and <Redirect />, a ternary test on route.props.path OR route.props.to is in my opinion sufficient. (I don't see what can be added beside Route and Redirect for client routing)

I am not familiar with the testing of the router, but testing if the redirect work is like testing the <Redirect /> itself and look ugly: https://github.com/ReactTraining/react-router/blob/master/packages/react-router/docs/guides/testing.md

Perhaps check if the <Resource /> is renderer is easier and would do the same.

[fzaninotto]

Can't we clone the routes given by the developer instead of creating new ones and passing the developer's props to them?

[kopax]

Do you mean this:

 customRoutes.filter(route => route.props.noLayout)
                        .map((route, index) => route.props.path ? (
                            <Route
                                key={index}
                                exact={route.props.exact}
                                path={route.props.path}
                                render={props =>
                                    this.renderCustomRoutesWithoutLayout(
                                        route,
                                        props
                                    )
                                }
                            />
                        ) :  cloneElement(route, route.props))

I was asking myself the same question for the <Route /> implementation the line before.

What makes you prefer one on another (for each case)? We could spread the user props and keep the same convention as for <Route />.

[fzaninotto]

No, I mean don't even bother to test for the path prop, clone the route directly. Whether it's a Route or a Redirect, it should work.

[kopax]

Sure, that's sounds better.

In this case, do you want to :

1. force exact .
2. keep it as a default.
3. let the user choose.

In case we do (2), should we remove it for <Redirect /> and keep it for the rest?

I've updated the PR with those change:

CoreAdminRoute (instead of):

{customRoutes
                        .filter(route => route.props.noLayout)
                        .map((route) => cloneElement(route, {
                          ...route.props ,
                          exact: true,
                          render: props =>
                            this.renderCustomRoutesWithoutLayout(
                              route,
                              props
                            )
                        }))}

RoutesWithLayout (instead of):

// instead of 
// {customRoutes &&
//                customRoutes.map((route) => cloneElement(route, route.props))}
// just 
{customRoutes}

• For CoreAdminRoute, is it voluntary to force the user to use exact=true, should it also concern the redirect? I assumed yes. Otherwise we should update the PR using for example this condition: exact: !!props.path || props.exact
• For RoutesWithLayout, I assumed it was better to append customRoutes instead of cloning.

[djhi]

The .ideadirectory and the package-lock.json are still present. Can you remove them?

[kopax]

Sorry, I just did it again. I had an issue when doing it the first time:

Would it be possible to run and add in this commit:

1. echo "/.idea" >> .gitignore
2. echo "package-lock=false" > .npmrc

• (1) will allow using Idea IntelliJ without worrying of pushing on the remote the .idea.
• (2) will prevent generating the package-lock.json to everyone. It is different than adding to .gitignore the package-lock.json, because no one will be able to have a lock file.

There's two options, either react-admin is a framework, or it is an application if it's a framework, it makes sense to not have a lock file as this is for an application.

[fzaninotto]

1. No, we won't search all possible IDEs and add all the files that they create in a project into our .gitignore. It's your IDE, it's your job to manage its files. IntelliJ is no exception.
2. Sorry, we won't do that either. If you want to contribute to react-admin, please use the tools we use, or assume the consequences. Once again, we can't support every tool chain in the world, so we chose yarn. I don't want to learn about package.lock and npmrc, I prefer to spend that time actually improving react-admin.

React-admin is a library, but that's not what motivates us to add a lock file. It's the fact that dependency management is permissive in JS, and a lock file is the only guarantee we have that something that worked yesterday still works today.

Also, this discussion has nothing to see with your original PR, so if you want to continue it, I suggest you open a new RFC issue.

[fzaninotto]

Thanks!

[kopax]

@fzaninotto thanks to you too for accepting this feature.