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

Get rid of eval #1611

Closed
wants to merge 26 commits into from
Closed
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
a0945c7
Introduce script tag based page loading system.
arunoda Apr 3, 2017
8938843
Call ensurePage only in the dev mode.
arunoda Apr 3, 2017
c95d2b2
Implement router using the page-loader.
arunoda Apr 4, 2017
e3d68ff
Fix a typo and remove unwanted code.
arunoda Apr 4, 2017
03209d8
Fix some issues related to rendering.
arunoda Apr 4, 2017
a1f11a4
Fix production tests.
arunoda Apr 5, 2017
6f65a05
Fix ondemand test cases.
arunoda Apr 5, 2017
57e3a67
Fix unit tests.
arunoda Apr 5, 2017
822a99b
Get rid of eval completely.
arunoda Apr 5, 2017
76bfc38
Remove all the inline code.
arunoda Apr 5, 2017
5aa02d0
Remove the json-pages plugin.
arunoda Apr 6, 2017
32cdf3f
Rename NEXT_PAGE_LOADER into __NEXT_PAGE_LOADER__
arunoda Apr 6, 2017
65a2603
Rename NEXT_LOADED_PAGES into __NEXT_LOADED_PAGES__
arunoda Apr 6, 2017
f6f175d
Remove some unwanted code.
arunoda Apr 6, 2017
e46edc2
Load everything async.
arunoda Apr 6, 2017
751d2e9
Remove lib/eval-script.js
arunoda Apr 6, 2017
4f26e84
Move webpack idle wait code to the page-loader.
arunoda Apr 6, 2017
866319c
Merge with master.
arunoda Apr 6, 2017
6f9b51f
Remove pageNotFound key from the error.
arunoda Apr 11, 2017
b5bd0da
Remove unused error field 'buildError'
arunoda Apr 11, 2017
c077ebe
Add much better logic to normalize routes.
arunoda Apr 11, 2017
fb2f90b
Get rid of mitt.
arunoda Apr 11, 2017
7997c1f
Introduce a better way to register pages.
arunoda Apr 11, 2017
6e0e7b4
Came back to the mitt() based page-loader.
arunoda Apr 11, 2017
95558ae
Add link rel=preload support.
arunoda Apr 12, 2017
6f735ee
Fix a typo.
arunoda Apr 12, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 24 additions & 7 deletions client/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@ import mitt from 'mitt'
import HeadManager from './head-manager'
import { createRouter } from '../lib/router'
import App from '../lib/app'
import evalScript from '../lib/eval-script'
import { loadGetInitialProps, getURL } from '../lib/utils'
import ErrorDebugComponent from '../lib/error-debug'
import PageLoader from '../lib/page-loader'

// Polyfill Promise globally
// This is needed because Webpack2's dynamic loading(common chunks) code
Expand All @@ -19,21 +19,35 @@ if (!window.Promise) {

const {
__NEXT_DATA__: {
component,
errorComponent,
props,
err,
pathname,
query
query,
buildId
},
location
} = window

const Component = evalScript(component).default
const ErrorComponent = evalScript(errorComponent).default
const pageLoader = window.NEXT_PAGE_LOADER = new PageLoader(buildId)
if (window.NEXT_LOADED_PAGES) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Prefix these globals with "__"

window.NEXT_LOADED_PAGES.forEach((fn) => fn())
delete window.NEXT_LOADED_PAGES
}

const ErrorComponent = pageLoader.loadPageSync('/_error')
let Component

try {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Load these in async and the initial scripts can be async.

Component = pageLoader.loadPageSync(pathname)
} catch (err) {
console.error(`${err.message}\n${err.stack}`)
Component = ErrorComponent
}

let lastAppProps

export const router = createRouter(pathname, query, getURL(), {
pageLoader,
Component,
ErrorComponent,
err
Expand All @@ -57,7 +71,10 @@ export default () => {
}

export async function render (props) {
if (props.err) {
// There are some errors we should ignore.
// Next.js rendering logic knows how to handle them.
// These are specially 404 errors
if (props.err && !props.err.ignore) {
await renderError(props.err)
return
}
Expand Down
11 changes: 6 additions & 5 deletions lib/error.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,15 @@ import HTTPStatus from 'http-status'
import Head from './head'

export default class Error extends React.Component {
static getInitialProps ({ res, jsonPageRes }) {
const statusCode = res ? res.statusCode : (jsonPageRes ? jsonPageRes.status : null)
return { statusCode }
static getInitialProps ({ res, err }) {
const statusCode = res ? res.statusCode : (err ? err.statusCode : null)
const pageNotFound = statusCode === 404 || (err ? err.pageNotFound : false)
return { statusCode, pageNotFound }
}

render () {
const { statusCode } = this.props
const title = statusCode === 404
const { statusCode, pageNotFound } = this.props
const title = pageNotFound
? 'This page could not be found'
: HTTPStatus[statusCode] || 'An unexpected error has occurred'

Expand Down
95 changes: 95 additions & 0 deletions lib/page-loader.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
/* global window, document */
import mitt from 'mitt'

export default class PageLoader {
constructor (buildId) {
this.buildId = buildId
this.pageCache = {}
this.pageLoadedHandlers = {}
this.registerEvents = mitt()
this.loadingRoutes = {}
}

normalizeRoute (route) {
if (route[0] !== '/') {
throw new Error('Route name should start with a "/"')
}

return route.replace(/index$/, '')
Copy link
Contributor

Choose a reason for hiding this comment

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

/foo/index and /foo would become different routes. Is it intended ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. Both are the same. Need to fix this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

}

loadPageSync (route) {
route = this.normalizeRoute(route)
const cachedPage = this.pageCache[route]

if (!cachedPage) {
return null
} else if (cachedPage.error) {
throw cachedPage.error
} else {
return cachedPage.page
}
}

loadPage (route) {
route = this.normalizeRoute(route)

const cachedPage = this.pageCache[route]
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks cache is not used when multiple concurrent requests happen for a route at a same time. maybe we can improve that later tho.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's doable. I'll do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, had to change this with the new mitt() less implementation.

if (cachedPage) {
return new Promise((resolve, reject) => {
if (cachedPage.error) return reject(cachedPage.error)
return resolve(cachedPage.page)
})
}

return new Promise((resolve, reject) => {
const fire = ({ error, page }) => {

Choose a reason for hiding this comment

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

Nice, this is cleaner using mitt than doing it manually

this.registerEvents.off(route, fire)

if (error) {
reject(error)
} else {
resolve(page)
}
}

this.registerEvents.on(route, fire)

// Load the script if not asked to load yet.
if (!this.loadingRoutes[route]) {
this.loadScript(route)
this.loadingRoutes[route] = true
}
})
}

loadScript (route) {
route = this.normalizeRoute(route)

const script = document.createElement('script')
const url = `/_next/${encodeURIComponent(this.buildId)}/page${route}`
script.src = url
script.type = 'text/javascript'
script.onerror = () => {
const error = new Error(`Error when loading route: ${route}`)
this.registerEvents.emit(route, { error })
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks we can ditch event emitter and make things simpler like the following.

return new Promise (resolve, reject) {
  script.onload = () => {
    const { page, statusCode } = window.__NEXT_PAGES_[route]
    if (statusCode >= 400) {
      const err = new Error()
      err.statusCode = statusCode
      reject(err)
      return
    }
    
    // wait for webpack ready

    resolve(page)
  }

  script.onerror = () => {
    reject(const error = new Error(`Error when loading route: ${route}`))
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This would make the script of page components simple as well, since it doesn't need to wait for pageLoader ready.

const newContent = `
  window.__NEXT_PAGES_[${JSON.stringify(routename)}] = {
    statusCode: ${statusCode},
    page: ${content}
  }
`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Let's do this.

Copy link
Contributor Author

@arunoda arunoda Apr 11, 2017

Choose a reason for hiding this comment

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

I'd like to keep the next pages inside a variable of PageLoader. Anyway doing following is not exactly possible.

const newContent = `
  window.__NEXT_PAGES_[${JSON.stringify(routename)}] = {
    statusCode: ${statusCode},
    page: ${content}
  }
`

That's because, webpack is not available when the page is loading at first.
But I did the next best thing.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can move values to PageLoader when it's ready.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what currently we are doing. Here's how this looks now:

const newContent = `
  window.__NEXT_REGISTER_PAGE('${routeName}', function() {
    var comp = ${content}
    return { page: comp.default }
  })
`


document.body.appendChild(script)
}

// This method if called by the route code.
registerPage (route, error, page) {
route = this.normalizeRoute(route)

// add the page to the cache
this.pageCache[route] = { error, page }
this.registerEvents.emit(route, { error, page })
}

clearCache (route) {
route = this.normalizeRoute(route)
delete this.pageCache[route]
delete this.loadingRoutes[route]
}
}
114 changes: 44 additions & 70 deletions lib/router/router.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
import { parse, format } from 'url'
import mitt from 'mitt'
import fetch from 'unfetch'
import evalScript from '../eval-script'
import shallowEquals from '../shallow-equals'
import PQueue from '../p-queue'
import { loadGetInitialProps, getURL } from '../utils'
Expand All @@ -10,19 +8,23 @@ import { _notifyBuildIdMismatch } from './'
const webpackModule = module

export default class Router {
constructor (pathname, query, as, { Component, ErrorComponent, err } = {}) {
constructor (pathname, query, as, { pageLoader, Component, ErrorComponent, err } = {}) {
// represents the current component key
this.route = toRoute(pathname)

// set up the component cache (by route keys)
this.components = { [this.route]: { Component, err } }

// contain a map of promise of fetch routes
this.fetchingRoutes = {}
this.components = {}
// We should not keep the cache, if there's an error
// Otherwise, this cause issues when when going back and
// come again to the errored page.
if (Component !== ErrorComponent) {
this.components[this.route] = { Component, err }
}

// Handling Router Events
this.events = mitt()

this.pageLoader = pageLoader
this.prefetchQueue = new PQueue({ concurrency: 2 })
this.ErrorComponent = ErrorComponent
this.pathname = pathname
Expand Down Expand Up @@ -77,7 +79,7 @@ export default class Router {

async reload (route) {
delete this.components[route]
delete this.fetchingRoutes[route]
this.pageLoader.clearCache(route)

if (route !== this.route) return

Expand Down Expand Up @@ -186,11 +188,11 @@ export default class Router {
try {
routeInfo = this.components[route]
if (!routeInfo) {
routeInfo = await this.fetchComponent(route, as)
routeInfo = { Component: await this.fetchComponent(route, as) }
}

const { Component, err, jsonPageRes } = routeInfo
const ctx = { err, pathname, query, jsonPageRes }
const { Component } = routeInfo
const ctx = { pathname, query }
routeInfo.props = await this.getInitialProps(Component, ctx)

this.components[route] = routeInfo
Expand All @@ -199,13 +201,27 @@ export default class Router {
return { error: err }
}

if (err.buildIdMismatched) {
// Now we need to reload the page or do the action asked by the user
_notifyBuildIdMismatch(as)
// We also need to cancel this current route change.
// We do it like this.
err.cancelled = true
return { error: err }
}

if (err.pageNotFound) {
// Indicate main error display logic to
// ignore rendering this error as a runtime error.
err.ignore = true
}

const Component = this.ErrorComponent
routeInfo = { Component, err }
const ctx = { err, pathname, query }
routeInfo.props = await this.getInitialProps(Component, ctx)

routeInfo.error = err
console.error(err)
}

return routeInfo
Expand Down Expand Up @@ -268,28 +284,7 @@ export default class Router {
cancelled = true
}

const jsonPageRes = await this.fetchRoute(route)
let jsonData
// We can call .json() only once for a response.
// That's why we need to keep a copy of data if we already parsed it.
if (jsonPageRes.data) {
jsonData = jsonPageRes.data
} else {
jsonData = jsonPageRes.data = await jsonPageRes.json()
}

if (jsonData.buildIdMismatch) {
_notifyBuildIdMismatch(as)

const error = Error('Abort due to BUILD_ID mismatch')
error.cancelled = true
throw error
}

const newData = {
...await loadComponent(jsonData),
jsonPageRes
}
const Component = await this.fetchRoute(route)

if (cancelled) {
const error = new Error(`Abort fetching component for route: "${route}"`)
Expand All @@ -301,7 +296,7 @@ export default class Router {
this.componentLoadCancel = null
}

return newData
return Component
}

async getInitialProps (Component, ctx) {
Expand All @@ -324,24 +319,22 @@ export default class Router {
return props
}

fetchRoute (route) {
let promise = this.fetchingRoutes[route]
if (!promise) {
promise = this.fetchingRoutes[route] = this.doFetchRoute(route)
async fetchRoute (route) {
// Wait for webpack to became idle if it's not.
// More info: https://github.com/zeit/next.js/pull/1511
if (webpackModule && webpackModule.hot && webpackModule.hot.status() !== 'idle') {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we need to do this inside the pageLoader before it run the code.

await new Promise((resolve) => {
const check = (status) => {
if (status === 'idle') {
webpackModule.hot.removeStatusHandler(check)
resolve()
}
}
webpackModule.hot.status(check)
})
}

return promise
}

doFetchRoute (route) {
const { buildId } = window.__NEXT_DATA__
const url = `/_next/${encodeURIComponent(buildId)}/pages${route}`

return fetch(url, {
method: 'GET',
credentials: 'same-origin',
headers: { 'Accept': 'application/json' }
})
return await this.pageLoader.loadPage(route)
}

abortComponentLoad (as) {
Expand All @@ -365,22 +358,3 @@ export default class Router {
function toRoute (path) {
return path.replace(/\/$/, '') || '/'
}

async function loadComponent (jsonData) {
if (webpackModule && webpackModule.hot && webpackModule.hot.status() !== 'idle') {
await new Promise((resolve) => {
const check = (status) => {
if (status === 'idle') {
webpackModule.hot.removeStatusHandler(check)
resolve()
}
}
webpackModule.hot.status(check)
})
}

const module = evalScript(jsonData.component)
const Component = module.default || module

return { Component, err: jsonData.err }
}
2 changes: 1 addition & 1 deletion server/build/plugins/json-pages-plugin.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ export default class JsonPagesPlugin {

pages.forEach((pageName) => {
const page = compilation.assets[pageName]
delete compilation.assets[pageName]
// delete compilation.assets[pageName]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove this plugin.


const content = page.source()
const newContent = JSON.stringify({ component: content })
Expand Down
Loading