fridays / next-routes

Universal dynamic routes for Next.js
MIT License
2.47k stars 230 forks source link

Should we avoid mutating the next/router? #77

Closed oliviertassinari closed 7 years ago

oliviertassinari commented 7 years ago

I have ran into an issue where next-routes is called twice. It's a mistake on my end, I have multiples routes to handle different subdomains. I'm supposed to always import the routes of a the current subdomain.

However, a slight import mistake has wide side effect. I think that we have an opportunity to reduce the impact of such possible mistake. What's the root cause? I have finally found that this modules is mutating next/router.

What do you think of stopping doing such? I mean returning a new object reference.

Also, I'm gonna add a safe defensive code on my end to throw when the next-routes is called a second time.

oliviertassinari commented 7 years ago

For anyone interested, here his my self defensive code:

safe.js

import NextRouter from 'next/router'

export default function safe(subdomain) {
  if (process.browser && NextRouter.subdomain && NextRouter.subdomain !== subdomain) {
    throw new Error(`Leak between ${subdomain} and ${NextRouter.subdomain}`)
  }

  NextRouter.subdomain = subdomain
}

www/routes.js

import nextRoutes from 'next-routes'
import safe from './safe'

safe('www')

const routes = nextRoutes()
routes.add('www/home', '/')
// ...

export const Router = routes.Router
export const Link = routes.Link

export default routes

admin/routes.js

import nextRoutes from 'next-routes'
import safe from './safe'

safe('admin')

const routes = nextRoutes()
routes.add('admin/home', '/')
// ...

export const Router = routes.Router
export const Link = routes.Link

export default routes
fridays commented 7 years ago

Thanks, I agree it would be better to not modify it. It was it's own object before, but there was a problem with Router events: https://github.com/fridays/next-routes/pull/18 If you could send a PR that solves this would be really cool.

oliviertassinari commented 7 years ago

I'm fine with the checks I have added in userland, I need them independently of this issue. It's not a priority for me. Feel free to close the issue.