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

Implement the Singleton Router API #429

Merged
merged 6 commits into from
Dec 19, 2016
Merged

Conversation

arunoda
Copy link
Contributor

@arunoda arunoda commented Dec 19, 2016

With this we could easily access the Router via next/router and work with it directly.
Here's a basic example:

pages/index.js

import Router from 'next/router'

const routeTo(href) {
  return (e) => {
    e.preventDefault()
    Router.push(href)
  }
}

export default () => (
  <div>Click <a href='#' onClick={routeTo('/about')}>here</a> to read more</div>
)

pages/about.js

export default () => (
  <p>Welcome to About!</p>
)

Above Router object as an API like this:

  • route - String of the current route
  • pathname - String of the current path excluding the query string
  • query - Object with the parsed query string. Defaults to {}
  • push(url, pathname=url) - performs a pushState call associated with the current component
  • replace(url, pathname=url) - performs a replaceState call associated with the current component

Usually, route is the same as pathname.
But when used with programmatic API, route and pathname can be different.
"route" is your actual page's path while "pathname" is the path of the url mapped to it.

Likewise, url and path is the same usually.
But when used with programmatic API, "url" is the route with the query string.
"pathname" is the path of the url mapped to it.

Example app is located at the examples/using-router

@@ -1,9 +1,8 @@
import { createElement } from 'react'
import { render } from 'react-dom'
import HeadManager from './head-manager'
import domready from 'domready'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we don't have the CDN setup, there's no need of using domready.
By the time of this script execution, we can access the __next div.

@coveralls
Copy link

coveralls commented Dec 19, 2016

Coverage Status

Coverage increased (+2.9%) to 60.27% when pulling 20d4eac on arunoda:singleton-router into 955f681 on zeit:master.

return router
}

// Export the actual Router class, which is also use internally
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently we don't use this anymore.
May be we could remove it.

@@ -97,11 +97,11 @@ export default class Router {
window.history.back()
}

push (route, url) {
push (route, url = route) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set the default value for url. So, we can access it like Router.push() if we don't change paths with #310

@@ -56,10 +56,11 @@ async function doRender (req, res, pathname, query, {
if (res.finished) return

const renderPage = () => {
const router = createRouter(pathname, query)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a safe call as React does rendering the same event loop.

//
// ## Server Side Usage
// We create router for every SSR page render.
// Since rendering happens in the same eventloop this works properly.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this something we can't avoid ? I want it work fine without any caveats.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, It seems we can't if we keep the syntax.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this is fine for React's case. And it's not going to change in the near future. So we are good to go.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed this wouldn't work on getInitialProps. I wonder if we should accept this too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use this inside getInitialProps is bad. We need to prevent this or give a fix.
I'll think about something.

Do you have any idea?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No :(

Additionally this doesn't work on custom Document (not only on getInitialProps) and custom server too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's discuss here: #443
I made a solution for getInitialProps. I'll think about inside the custom Document.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants