Get rid of eval #1611

arunoda · 2017-04-04T03:51:08Z

Got a lot of ideas (almost all actually) from @bringking #1608
The difference is, I wanted to make this minimal as possible and incrementally integrate into Next.js

bringking · 2017-04-04T05:01:09Z

lib/page-loader.js

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


Nice, this is cleaner using mitt than doing it manually

bringking · 2017-04-04T05:08:25Z

server/index.js

+          await this.hotReloader.ensurePage(pathname)
+        }
+
+        if (!this.handleBuildId(params.buildId, res)) {


This makes sense, however since buildId is in the URL segment, do you think it would make more sense to 404 on a buildId mismatch? Conceptually isn't a buildId mismatch just a missing resource? I think that would cause the loadScript -> <script> onerror to fire, and then your error listener would pick it up.

@bringking That's true.
But detecting 404 between browsers will be an issue.
This is a much better way and we know it's working 100% sure.

And we could improve this later on.

arunoda · 2017-04-05T12:23:17Z

lib/router/router.js

+  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') {


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

arunoda · 2017-04-05T12:24:07Z

server/build/plugins/json-pages-plugin.js

@@ -7,7 +7,7 @@ export default class JsonPagesPlugin {

      pages.forEach((pageName) => {
        const page = compilation.assets[pageName]
-        delete compilation.assets[pageName]
+        // delete compilation.assets[pageName]


Remove this plugin.

arunoda · 2017-04-05T12:25:05Z

server/build/plugins/pages-plugin.js

+        // TODO: We need to move "client-bundles" back to "bundles" once we remove
+        // all the JSON eval stuff
+        delete compilation.assets[chunk.name]
+        compilation.assets[`client-bundles/pages/${pageName}.js`] = {


Make the "client-bundles" back to "bundles".

arunoda · 2017-04-05T12:26:13Z

server/render.js

@@ -105,26 +103,70 @@ async function doRender (req, res, pathname, query, {
      err: (err && dev) ? errorToJSON(err) : null
    },
    dev,


We don't need these anymore.

arunoda · 2017-04-05T12:28:46Z

client/index.js

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


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

arunoda · 2017-04-05T12:29:31Z

client/index.js

-const Component = evalScript(component).default
-const ErrorComponent = evalScript(errorComponent).default
+const pageLoader = window.NEXT_PAGE_LOADER = new PageLoader(buildId)
+if (window.NEXT_LOADED_PAGES) {


Prefix these globals with "__"

bringking · 2017-04-05T18:00:53Z

server/document.js

@@ -97,11 +97,14 @@ export class NextScript extends Component {

  render () {


Not critical or a blocker for this approach to work, but I would love to see the addition of <link rel="preload"> to see if helps mitigate the added latency of downloading the components as script tags. Since we are not streaming the HTML, it might make negligible difference

I didn't get this, could you give me more info?

If you add a link tag with the attribute "preload" in the head, it gives the browser a hint that we are going to be loading some scripts later on in the document. This could possibly give us a slight boost in Time To Interactive. See this for some background. https://www.smashingmagazine.com/2016/02/preload-what-is-it-good-for/

So in the _document, for the initial components (errorComponent and the current route bundle) you could render and possibly have them evaluated sooner. But we should profile it and see if it helps. Does that make more sense?

@bringking I'll prefer using <script async /> even that article mentioned it's blocks.
In my tests and experience, browser will start painting the page before even the script hits the server.
(script async is not yet done)
But I'll do it.

Also, preload links would be helpful for any Link marked as "prefetch" since the browser can start those downloads right away. This possibility is now available since we are using plain JS (you can't preload JSON according to the spec)

@bringking I tested with very low network connections and script async do fine.
Since link preload only chrome now and script async is doing great for us, I'm going to defer this to later.

We no longer need it.

Because that's the place to do it.

arunoda · 2017-04-06T19:23:51Z

This is ready to 🚢

bringking

Not my place to approve, however in my testing this branch decreases our time-to-interactive by ~1-2 seconds on a 3G connection, since the HTML document is considerably smaller. And no un-safe eval to boot. Awesome job man!

Minor question - I noticed <Link prefetch> results in a non-async script tag. Should these be async as well?

bringking · 2017-04-06T20:20:22Z

client/index.js

+const pageLoader = window.__NEXT_PAGE_LOADER__ = new PageLoader(buildId)
+if (window.__NEXT_LOADED_PAGES__) {
+  window.__NEXT_LOADED_PAGES__.forEach((fn) => fn())
+  delete window.__NEXT_LOADED_PAGES__


Not sure if this matters anymore, but at some point it was considered "bad" to use delete since it is hard to optimize by V8 and other compilers. Whenever I see delete, usually I think there might be a better way via re-assignment. Obviously not a blocker, just an observation

Actually, delete is not bad. You need to use it wisely. Delete is bad when used inside a constructor. Because that's de-optimization. That's also valid for when we are creating a lot of objects from that class.

We need to use it wisely. And we need to delete when needed. Anyway, it's not needed here and having it also not a problem :)

arunoda · 2017-04-07T06:47:06Z

@bringking good question.
Seems like browsers do it automatically for us: http://stackoverflow.com/questions/3408805/is-the-async-attribute-property-useful-if-a-script-is-dynamically-added-to-the?answertab=active#tab-top

bringking · 2017-04-09T20:30:24Z

@arunoda this is awesome, any thoughts on when you guys can get this one out?

rauchg · 2017-04-10T05:44:14Z

@bringking this is high priority. Whenever @nkzawa can review this will ship

nkzawa · 2017-04-10T21:33:46Z

server/render.js

+      function loadPage () {
+        var error = new Error('Page not exists: ${page}')
+        error.pageNotFound = true
+        error.statusCode = 404


Why do we have two similar values ?

Good one. I think we can live with statusCode. I'll change it.

nkzawa · 2017-04-10T21:39:48Z

lib/page-loader.js

+      throw new Error('Route name should start with a "/"')
+    }
+
+    return route.replace(/index$/, '')


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

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

nkzawa · 2017-04-10T23:12:21Z

lib/page-loader.js

+    script.onerror = () => {
+      const error = new Error(`Error when loading route: ${route}`)
+      this.registerEvents.emit(route, { error })
+    }


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}`)) } }

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} } `

Good point. Let's do this.

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.

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

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 } }) `

nkzawa · 2017-04-10T23:14:51Z

lib/page-loader.js

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


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.

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

@nkzawa actually concurrent request are handled here: https://github.com/zeit/next.js/pull/1611/files/866319c76d0fa80df68d7aca809f3af81c68e6b6#diff-c7ddf3ad46bcb0938d6043ea0393d554R48

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

arunoda · 2017-04-11T14:35:09Z

@nkzawa updated based on your comments.

arunoda · 2017-04-11T16:26:56Z

EDIT: Fixed the bug.

Found a bug and fixing it.

arunoda · 2017-04-11T16:39:48Z

Had to came back to the mitt() based page loader. If we are rely on script.onload, we need to maintain few other states as we are doing some async ops.

Anyway, since we use mitt() in other places, this is fine.

arunoda · 2017-04-18T13:39:10Z

Closing this PR since #1700 has it.

arunoda added 2 commits April 3, 2017 23:40

Introduce script tag based page loading system.

a0945c7

Call ensurePage only in the dev mode.

8938843

arunoda mentioned this pull request Apr 4, 2017

[#1301] Save pages as JS, remove eval #1608

Closed

5 tasks

bringking reviewed Apr 4, 2017

View reviewed changes

jstcki mentioned this pull request Apr 4, 2017

Add next export for static builds #1576

Closed

arunoda added 6 commits April 5, 2017 01:25

Implement router using the page-loader.

c95d2b2

Fix a typo and remove unwanted code.

e3d68ff

Fix some issues related to rendering.

03209d8

Fix production tests.

a1f11a4

Fix ondemand test cases.

6f65a05

Fix unit tests.

57e3a67

sedubois mentioned this pull request Apr 5, 2017

Add example app with React Intl #1055

Merged

arunoda added 2 commits April 5, 2017 12:15

Get rid of eval completely.

822a99b

Remove all the inline code.

76bfc38

arunoda commented Apr 5, 2017

View reviewed changes

bringking reviewed Apr 5, 2017

View reviewed changes

arunoda mentioned this pull request Apr 6, 2017

app.js on slow connections #1630

Closed

arunoda added 7 commits April 6, 2017 10:33

Remove the json-pages plugin.

5aa02d0

Rename NEXT_PAGE_LOADER into __NEXT_PAGE_LOADER__

32cdf3f

Rename NEXT_LOADED_PAGES into __NEXT_LOADED_PAGES__

65a2603

Remove some unwanted code.

f6f175d

Load everything async.

e46edc2

Remove lib/eval-script.js

751d2e9

We no longer need it.

Move webpack idle wait code to the page-loader.

4f26e84

Because that's the place to do it.

bringking reviewed Apr 6, 2017

View reviewed changes

bringking mentioned this pull request Apr 6, 2017

2x size of the initial bundle load #1637

Closed

rauchg requested a review from nkzawa April 10, 2017 05:44

nkzawa reviewed Apr 10, 2017

View reviewed changes

arunoda added 5 commits April 11, 2017 14:51

Remove pageNotFound key from the error.

6f9b51f

Remove unused error field 'buildError'

b5bd0da

Add much better logic to normalize routes.

c077ebe

Get rid of mitt.

fb2f90b

Introduce a better way to register pages.

7997c1f

arunoda changed the title ~~Get rid of eval~~ [WIP] Get rid of eval Apr 11, 2017

Came back to the mitt() based page-loader.

6e0e7b4

arunoda changed the title ~~[WIP] Get rid of eval~~ Get rid of eval Apr 11, 2017

arunoda mentioned this pull request Apr 12, 2017

Add CDN support with assetPrefix #1700

Merged

arunoda added 2 commits April 12, 2017 16:11

Add link rel=preload support.

95558ae

Fix a typo.

6f735ee

arunoda closed this Apr 18, 2017

arunoda deleted the no-eval branch April 18, 2017 13:39

lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019

		@@ -97,11 +97,14 @@ export class NextScript extends Component {

		render () {

Get rid of eval #1611

Get rid of eval #1611

Conversation

arunoda commented Apr 4, 2017

Choose a reason for hiding this comment

bringking Apr 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bringking Apr 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arunoda commented Apr 6, 2017

bringking left a comment • edited Loading

Choose a reason for hiding this comment

bringking Apr 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arunoda commented Apr 7, 2017

bringking commented Apr 9, 2017

rauchg commented Apr 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arunoda Apr 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arunoda commented Apr 11, 2017

arunoda commented Apr 11, 2017 • edited Loading

arunoda commented Apr 11, 2017

arunoda commented Apr 18, 2017

bringking Apr 4, 2017 •

edited

Loading

bringking Apr 5, 2017 •

edited

Loading

bringking left a comment •

edited

Loading

bringking Apr 6, 2017 •

edited

Loading

arunoda Apr 11, 2017 •

edited

Loading

arunoda commented Apr 11, 2017 •

edited

Loading