Two private routes on the same line with **one** space in between will break your app

[esteban_url]

Hey folks, I discovered, by accident, a weird issue today, it’s described below, with a repo that shows the error, does this sound and looks like a good issue to be opened in github? Or would it just create noise.

I’m willing work on it to try to fix it, would need someone to point me in the right direction as this would be my first framework contribution.

Also I don’t really know who to create tests for it.

Description of the issue

• If you have two private routes on the same line with one space in between your application will break.
• It’s not picked up by prettier, so they stay on the same line.
• If multiples spaces are inserted between the two routes prettier will reduce them to only one space.
• If no spaces are in between the routes, prettier will put the second route in a new line, fixing the problem.
• The spaces behave the same way in "public routes, but the app doesn’t break.

Examples

this will break your app :

    <Router>
      <Route path="/signin" page={SigninPage} name="signin" />
      <Private unauthenticated="signin">
        {/* two private routes on the same line? 🚨🚨🚨🚨🚨 */}
        <Route path="/secret" page={SecretPage} name="secret" /> <Route path="/dashboard" page={DashboardPage} name="dashboard" />
      </Private>
      {/* two public routes on the same line? ✅✅✅✅✅ */}
      <Route path="/" page={HomePage} name="home" /> <Route notfound page={NotFoundPage} />
    </Router>

this works :

    <Router>
      <Route path="/signin" page={SigninPage} name="signin" />
      <Private unauthenticated="signin">
        <Route path="/secret" page={SecretPage} name="secret" />
        <Route path="/dashboard" page={DashboardPage} name="dashboard" />
      </Private>
      {/* two public routes on the same line? ✅✅✅✅✅ */}
      <Route path="/" page={HomePage} name="home" /> <Route notfound page={NotFoundPage} />
    </Router>

Browser console output

Example repo:

https://github.com/esteban-url/rw-route-example

Running example (not running I guess)

https://wonderful-bardeen-1a371b.netlify.app/

[esteban_url]

I dug into the router code, created a test that replicates the issue, and devised a fix for it.

Should a create an issue and a PR for it?

proposed test:

it('inits routes two private routes with a space in between and loads as expected', async () => {
  const TestRouter = () => (
    <Router>
      <Route path="/" page={HomePage} name="home" />
      <Route path="/about" page={AboutPage} name="about" />
      <Route path="/redirect" page={RedirectPage} name="redirect" />
      <Private unauthenticated="home">
        <Route path="/private" page={PrivatePage} name="private" />{' '}
        <Route path="/another-private" page={PrivatePage} name="private" />
      </Private>

      <Route
        path="/param-test/:value"
        page={({ value }) => <div>param {value}</div>}
        name="params"
      />
    </Router>
  )
  const screen = render(<TestRouter />)

  // starts on home page
  await waitFor(() => screen.getByText(/Home Page/i))
})

proposed fix (without the comment ):

  const routes = React.useMemo(() => {
    // Find `Private` components, mark their children `Route` components as private,
    // and merge them into a single array.
    const privateRoutes =
      React.Children.toArray(children)
        .filter((child) => child.type === Private)
        .map((privateElement) => {
          // Set `Route` props
          const { unauthenticated, role, children } = privateElement.props
          return React.Children.toArray(children)
//this filter does the trick✨✨✨
            .filter((x) => typeof x === 'object')
            .map((route) =>
              React.cloneElement(route, {
                private: true,
                unauthenticatedRedirect: unauthenticated,
                role: role,
              })
            )
        })
        .reduce((a, b) => a.concat(b), []) || []

    const routes = [
      ...privateRoutes,
      ...React.Children.toArray(children).filter(
        (child) => child.type === Route
      ),
    ]

    return routes
  }, [children])

[Tobbe]

Great work @esteban_url Please do create an issue + PR

[thedavid]

Thanks @esteban_url!

That was some rapid work from diagnosis to PR to merged code

[esteban_url]

Thanks @thedavid! that introduction to contributing the other day really helped!

[thedavid]

Glad to hear! 'Cause I still feel guilty for that “30 min demo” == 1 hour. Anyway, you were a champion and it really helped the process to have you be included. So thanks again.