-
Notifications
You must be signed in to change notification settings - Fork 27.9k
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
Conversation
@@ -1,9 +1,8 @@ | |||
import { createElement } from 'react' | |||
import { render } from 'react-dom' | |||
import HeadManager from './head-manager' | |||
import domready from 'domready' |
There was a problem hiding this comment.
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.
return router | ||
} | ||
|
||
// Export the actual Router class, which is also use internally |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
With this we could easily access the Router via
next/router
and work with it directly.Here's a basic example:
pages/index.js
pages/about.js
Above
Router
object as an API like this:route
-String
of the current routepathname
-String
of the current path excluding the query stringquery
-Object
with the parsed query string. Defaults to{}
push(url, pathname=url)
- performs apushState
call associated with the current componentreplace(url, pathname=url)
- performs areplaceState
call associated with the current componentExample app is located at the
examples/using-router