ironhack-edu/ironlauncher

Update to React Router DOM v6 is not straightforward

Closed this issue ยท 5 comments

Hello there.
Updating to v6 of react-router-dom brings up a question in architecture.

Previously we were using a concept of Normal vs Protected routing components. However, that structure is no longer accepted, because React-Router-Dom's Routes component only accepts Route components as it's direct children.

So: this is not acceptable:

function Normal() {
  return <Route path="/" element={<HomePage />} />
}

This is not accepted because even though Normal returns a Route component. It is, indeed, not a Route Component.

With this issue, a couple of solutions are created. For fairness I like some more than others

1 - โš ๏ธ ๐Ÿ›‘ I DONT LIKE THIS Using React.Context to share all necessary props (like user). Cleaner, probably what I would use in my own app (or any other global context provider, with a combination of the third), but it would be another thing the students would have to understand

2 - This one, might be straightforward, but there is a looot of code duplication, and I dont know if this can have any performance issues with nested Routes

function Normal(props){
  return (
    <Routes>
      <Route {...props} />
    </Routes>
  )
}

function Protected(props){
  if (!props.user) {
    return <Navigate to="/" replace /> // Navigate is the new Redirect
  }
  return (
    <Routes>
      <Route {...props } />
    </Routes>
  )
}

Every one of these routes would, then, have to be siblings of each others.

Something like:

function App(){
  return (
    <Normal path="/" element={<Home />} />
    <Protected user={user} element={<VeryPrivatePage />} />
  )
}

3 - I like this the most, more abstracted, might be harder to visualize. More maintainable, but possibly harder to grasp :

// could be in a config/routes.js
const routes = (props) => [
    {
      path: "/",
      element: <HomePage {...props} />
    },
    {
      path: "/protected",
      element: props.user ? <ProtectedRoute {...props} />  : <Navigate to="/" replace />
    }
]

// now in App.js of example

function App(){
  const [user, setUser] = useState({})
  
  function authenticate(newUser) {
    setUser(newUser)
  }

  return (
  // here whatever markup the students would want
    <Routes>
        {routes({ user, authenticate }}.map(routeConfig => <Route path={routeConfig.path} element={routeConfig.element} /> )} 
        {/* Another solution, cleaner even -> <Route {...routeConfig} /> */ }
    </Routes>
  )
}

PROS AND CONS OF 1, 2 and 3 (Personal opinion)

1 - Context

Pros:

  • Fancy new syntax
  • Powerful

Cons:

  • Fancy new syntax
  • Too abstracted, in my opinion

2 - Sibling + Routes

Pros:

  • Easier, i believe, to see and to follow
  • It's just a wrapper around the route itself. The Normal is basically obsolete. It's just there to wrap the Routes

Cons:

  • Too many Routes.
  • Looks weird and just wrong
  • Feels dirty, dont know why.

3 - Routes function

Pros:

  • Fancy configuration variable
  • Maintainable
  • Closer to what students will find in the market

Cons:

  • Fancy configuration variable
  • Might be too abstracted from the students
  • Might cause a lot more noise in the beginning and cause more confusion

Personally I feel a little confused. If it was up to me I would show the version 3 because it's closer to what I've seen in the market and would just show students already a good solution, but I can see arguments against it, obviously.

Looking for some feedback!

Although I can understand that Solution 3 would introduce a new layer of abstraction on the routes side, it would be also my choice. It's closer to what you'd see out in the market and it provides good scalability & structure for the projects. Those advantages are important enough for me that it would justify taking class time to explain its structure. โœŒ

I would prefer to use Solution 3 but in the course of the explanation i would also want to show Solution 2.

3 is what we would want them to eventually code in but I'd go for solution 2 to start off with. Then probably introduce 3, since it would be quite hard to grasp for most newcomers in coding.

After trying to go through option 2 today in class I noticed that it is much harder to debug.
Its fair that you just have set it up once but I actually believe option 3 might be easier to reason about in the long run.

After class I actually tried running it again on my own and I can tell you that it took me 20 more minutes just to figure out where the issue was.

Also, with option 3 we can even teach an option where the main differentiator is not only user, but for example an admin role.

So I think I will implement option 3 for now. I can add another flag eventually to have the other option.

cc: @ManishPoduval @J-1000 @filipe-freire

Thanks for the quick fix @itstheandre