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

routeConfig array breaks with nested children, unless an empty object is provided #657

Open
santino opened this issue Jan 30, 2020 · 4 comments

Comments

@santino
Copy link

santino commented Jan 30, 2020

Hi @taion,
I have recently upgraded to found 0.4.9 and bumped into quite an annoying issue.

I suppose most of people create a router by using the JSX route config which still works perfectly fine.
However when using the routeConfig array of objects this no longer works as documented and I'm not able to navigate through routes that have a children.

I noticed that when using the JSX routes, makeRouteConfig will prepend an empty object to all the children routes.
The issue lies here, routeConfig array now expects the empty object to be defined when you have a children; most likely to cater for the scenario where you want to render a component on the children without the need of a subpath; f.i. on the top level route as follow:

const routeConfig = [{ path: '/', Component: AppPage, children: [{ Component: MainPage, }, {...}, {...}] }]

To summarise as per documentation this is how we're supposed to build the routeConfig array of objects:

const routeConfig = [
  {
    path: '/',
    Component: AppPage,
    children: [
      {
        Component: MainPage,
      },
      {
        path: 'foo',
        Component: FooPage,
        children: [
          {
            path: 'bar',
            Component: BarPage,
          },
        ],
      },
    ],
  },
];

However the above won't allow navigation through children whilst instead adding an empty object to the children array will fix this issue; as below:

const routeConfig = [
  {
    path: '/',
    Component: AppPage,
    children: [
      {},
      {
        Component: MainPage,
      },
      {
        path: 'foo',
        Component: FooPage,
        children: [
          {},
          {
            path: 'bar',
            Component: BarPage,
          },
        ],
      },
    ],
  },
];

I believe this wasn't an intentional breaking change as is not documented, and I'm not sure it would make much sense, so should probably be classified as a bug.

Any chance you can issue a patch for this?

@taion
Copy link
Contributor

taion commented Jan 31, 2020

This was a breaking change that was added in v0.4.0 in #217. The idea is that, without this behavior, it was impossible to make /foo not match. It was possible to opt into this behavior from v0.3.12 onward.

This is the same between both JSX route configs and object route configs.

Does that make sense? Was the issue that the release notes didn't describe clearly enough what this change was?

@santino
Copy link
Author

santino commented Jan 31, 2020

Thanks for the answer.
Yes, I definitely didn't get this following the release notes.

The fact that an empty object is required is not really mentioned and also all the examples of routeConfig array of objects in the README file do not show any empty object.
So anyone migrating or even someone new wouldn't know what is wrong as this doesn't seem to be a requirement.

Definitely not a problem when using JSX given makeRouteConfig will add that automatically; but you need to add it to your array of objects manually when not using JSX and so need to be aware.

EDIT: if the empty object is now a requirement, why not processing the received array of object within the library to add it rather than forcing users to have a breaking requirement?
This functionality is already provided on the JSX by makeRouteConfig, why not making it available to the routeConfig array of objects?

@taion
Copy link
Contributor

taion commented Jan 31, 2020

To be clear, makeRouteConfig does not add this automatically. If you're using JSX, you need to go from:

<Route path="foo">
  <Route path="bar" />
</Route>

to

<Route path="foo">
  <Route />
  <Route path="bar" />
</Route>

Assuming we're talking about the same thing here.

@santino
Copy link
Author

santino commented Feb 1, 2020

You're absolutely right.
I've got mistaken because the issue I'm having is with my storybook routeConfig where I'm using the array of objects, which got broken following the upgrade.
I didn't change, instead, my App Router and so I was under the impression that makeRouteConfig did cater for this but looking back at my router I do indeed have a <Route> child (with Component, query, prepareVariables etc.) under my <Route path='foo'> .

Sorry for the confusion on this point; good to hear that the behaviour of the library is consistent between the JSX and the array of objects route config.

I still believe that if you are using array of objects it would be quite odd having to add an empty object to your children array.
But if this is the intended behaviour and, you probably know better than me that, it doesn't really represent an issue given most of users will go for the JSX config; then I suppose you won't see many reasons to post-process the array of objects to eventually add the empty object when not there.

In this case I guess the only action out of this ticket would be to update the README to make sure the empty children is documented in your examples.

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

No branches or pull requests

2 participants