Prerendering #3359

k88hudson · 2017-09-06T17:38:59Z

This patch adds prerendering in a few commits:

a5356f7: This is just some basic refactoring to not crash in node (using global for globals, etc.)
2193572: This changes the state rehydration to work with a new action, NEW_TAB_STATE_REQUEST, instead of NEW_TAB_LOAD. This is needed for prerendering in order to ensure the first render matches the prerendered state.
45357fb: This adds the buildmc:html task, and all the basic prerendering tasks.
686518a: This adds a pref to determine whether or not the pre-rendered version should be used. I need to actually add a patch to aboutNewTabService to make use of this.
209f43dd: This is a small change to <Topics /> to allow it to render but be empty. This is to ensure things don't move around when the pre-rendered version switches to React.

dmose · 2017-09-06T21:27:49Z

system-addon/common/PerfService.jsm

+  usablePerfObj = {
+    now() {},
+    mark() {}
+  };


I'm assuming this is needed so that it runs under node. A comment indicating that would nice.

dmose · 2017-09-06T21:29:12Z

system-addon/content-src/lib/constants.js

@@ -1,4 +1,4 @@
 module.exports = {
  // constant to know if the page is about:newtab or about:home
-  IS_NEWTAB: document.documentURI === "about:newtab"
+  IS_NEWTAB: global.document && global.document.documentURI === "about:newtab"


What is it that keeps this code running in the browser? Webpack magic? Do we assign global=Window somewhere?

global is window in webpack, yes

dmose · 2017-09-06T21:30:43Z

system-addon/content-src/components/Base/Base.jsx

@@ -22,6 +23,9 @@ function addLocaleDataForReactIntl({locale}) {

 class Base extends React.Component {
  componentDidMount() {
+    // Request state after the first render
+    this.props.dispatch(ac.SendToMain({type: at.NEW_TAB_STATE_REQUEST}));
+


A comment explaining why we do this extra round trip (e.g. for pre-rendering), perhaps even in a bit of details, would likely be useful to future code readers.

A unit test for this would be good too; https://github.com/airbnb/enzyme/blob/master/docs/api/mount.md has an example using sinon that should be similar.

dmose · 2017-09-06T21:33:41Z

system-addon/lib/NewTabInit.jsm

@@ -15,7 +15,7 @@ this.NewTabInit = class NewTabInit {
  onAction(action) {
    let newAction;
    switch (action.type) {
-      case at.NEW_TAB_LOAD:
+      case at.NEW_TAB_STATE_REQUEST:


This wants a unit test, I think. Maybe a look at the code coverage numbers would show other changes that want one too?

Currently this file isn't being tested (which is an oversight), do you mind if I file a follow-up for this?

Because of the high-value in getting this into the tree sooner rather than later, yes.

dmose · 2017-09-06T21:41:19Z

bin/render-activity-stream-html.js

+
+function templateJs(state) {
+  return `// Note - this is a generated file.
+  window.gActivityStreamPrerenderedState = ${JSON.stringify(state, null, 2)};


This wants a comment describing what it's doing and why. Probably in the rendered file so that people who find that artifact quickly see what is.

dmose · 2017-09-06T21:42:27Z

bin/render-activity-stream-html.js

+`;
+}
+
+const args = require("minimist")(process.argv.slice(2), {alias: {baseUrl: "b", title: "t", locale: "l", direction: "d"}});


This is a bit hard to skim. Maybe break it up into two lines, and explain the magic constant of 2 in a comment?

dmose · 2017-09-06T21:43:39Z

bin/render-activity-stream-html.js

+fs.writeFileSync(HTML_FILE_PATH, templateHTML(options)); // eslint-disable-line no-sync
+fs.writeFileSync(PRERENDERED_HTML_FILE_PATH, templateHTML(options, html)); // eslint-disable-line no-sync
+fs.writeFileSync(INITIAL_STATE_JS_FILE_PATH, templateJs(state)); // eslint-disable-line no-sync
+


It'd improve readaibility to just put /* eslint-disable no-sync */ at the top of the file.

Mardak · 2017-09-06T22:28:26Z

system-addon/content-src/components/Base/Base.jsx

+    // Note: the key on IntlProvider must be static in order to not blow away
+    // all elements on a locale change (such as after preloading).
+    // See https://github.com/yahoo/react-intl/issues/695 for more info.
+    return (<IntlProvider key="STATIC" locale={locale} messages={strings}>


This change seems to cause the search placeholder and migration texts to not appear on the first about:home. (You can also test with ./mach run about:newtab) Changing back to key={locale} avoids this:

My guess is that LOCALE_UPDATED happens at an unexpected time? Also, guessing that TOP SITES gets the string because TopSites gets re-render-ed.

Looks like passing in a dummy locale prop, e.g., <Search locale={locale} />, makes it render again

Mardak · 2017-09-07T03:29:10Z

I pushed to talos moving the activity-stream-prerendered.html to activity-stream.html:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=c959327c6b75cd4930a6ea087583c38b805e7524&newProject=try&newRevision=099db27cebe866024186aac8f4f0274c60839d09&framework=1&filter=7%20opt

Nothing significant so far although tabpaint shows 6% improvement but low confidence after 10 before and 10 after runs.

Mardak · 2017-09-07T12:24:01Z

system-addon/data/content/activity-stream.html

-  <body class="activity-stream">
-    <div id="root"></div>
-    <div id="snippets-container">
-      <div id="snippets"></div>


We'll still need these snippets related div#ids, yeah?

Mardak · 2017-09-07T12:27:56Z

Still nothing significant (and maybe even a slight tart regression?)

I also pushed to try with aboutHome enabled:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=c959327c6b75cd4930a6ea087583c38b805e7524&newProject=try&newRevision=0c03e6c544e7a8a222b775b009110932d2a16cf9&framework=1

dmose · 2017-09-07T16:27:44Z

bin/render-activity-stream-html.js

+    <link rel="icon" type="image/svg+xml" href="${options.baseUrl}img/newtab-icon.svg">
+  </head>
+  <body>
+    <div id="root">${isPrerendered ? html : ""}</div>${isPrerendered ? `


Having so much of the semantics come from punctuation, and specifically nested template strings, makes this a bit hard to follow. Pull out the <script> for stream-initial-state into a const higher up in the script?

dmose · 2017-09-07T16:31:10Z

bin/render-activity-stream-html.js

@@ -0,0 +1,59 @@
+const fs = require("fs");
+const path = require("path");
+const prerender = require("./prerender");


Add a comment here about what this is and how it's built?

dmose · 2017-09-07T16:33:27Z

bin/render-activity-stream-html.js

+`;
+}
+
+function templateJs(state) {


JSDoc here for first-time readers would be helpful, mostly for the info about "state"

Very nice; thanks!

dmose · 2017-09-07T16:56:13Z

system-addon/content-src/activity-stream-prerender.jsx

+    Object.keys(allStrings["en-US"]).forEach(key => {
+      strings[key] = " ";
+    });
+  }


@Mardak, does this need a spin-off bug to properly do individual string fallback correctly?

I think string fallbacks work decently well at the moment (they fall back to English for any missing strings), although there are some cases where dynamic string ids get injected (e.g. TopStories) that could be handled better on a case-by-case basis

dmose · 2017-09-07T17:07:02Z

system-addon/content-src/activity-stream-prerender.jsx

+      topics: [{name: "Must Reads", category: "World", url: "https://getpocket.com/explore/must-reads?src=ff_new_tab"}]
+    }
+  });
+


Since each of the above dispatches effectively replaces ones that currently come from chrome, but have a comment above each one talking about how they differ or any assumptions in the interest of future code maintainability.

Particularly keeping in mind that effectively this code may also need to be updated as the chrome code changes over time.

It's not really so much that it comes from chrome as that specific actions are needed to set properties in certain reducers

I can actually add tests to help with this

That sounds good.

dmose · 2017-09-07T17:29:49Z

system-addon/content-src/activity-stream-prerender.jsx

@@ -0,0 +1,63 @@
+const React = require("react");


This file definitely wants unit tests.

dmose · 2017-09-07T17:31:02Z

system-addon/content-src/activity-stream.jsx


 new DetectUserSessionStart().sendEventOrAddListener();

-const store = initStore(reducers);
+const store = initStore(reducers, global.gActivityStreamPrerenderedState);


The changes to this file want unit tests too.

dmose · 2017-09-07T17:33:47Z

system-addon/content-src/lib/init-store.js

@@ -49,20 +49,23 @@ const messageMiddleware = store => next => action => {
 * @param  {object} reducers An object containing Redux reducers


There should probably be one or two unit tests added here.

dmose · 2017-09-07T17:36:57Z

system-addon/common/PrerenderData.jsm

+const prefConfig = {
+  // Prefs listed with "invalidates: true" will prevent the prerendered version
+  // of AS from being used if their value is something other than what is listed
+  // here.


As we were discussing yesterday, some of these have minor consequences, which we currently believe are acceptable. Documenting those consequences and tradeoffs here would help maintainability.

dmose · 2017-09-07T17:42:00Z

system-addon/test/unit/lib/PrefsFeed.test.js

+      feed.onPrefChanged("foo123", true);
+      assert.notCalled(feed._prefs.set);
+    });
+    it("should set the prerender pref if one of the prefs in PrerenderData.invalidatingPrefs is changed", () => {


This wants to be split into two tests for the true -> false change and the false -> true change.

dmose · 2017-09-07T17:46:10Z

bin/render-activity-stream-html.js

@@ -1,3 +1,4 @@
+ /* eslint-disable no-sync */  /* eslint-disable no-sync */  /* eslint-disable no-sync */


Three times? :-)

dmose · 2017-09-07T17:47:16Z

system-addon/content-src/components/Search/Search.jsx

@@ -82,5 +82,5 @@ class Search extends React.Component {
 }

 // initialized is passed to props so that Search will rerender when it receives strings


This comment needs to be updated too.

dmose · 2017-09-07T17:48:38Z

bin/render-activity-stream-html.js

@@ -29,6 +29,7 @@ const DEFAULT_OPTIONS = {
 */
 function templateHTML(options, html) {
  const isPrerendered = !!html;
+  const scriptForPrerendering = `\n<script src="${options.baseUrl}data/content/activity-stream-initial-state.js"></script>`;


nit: It'd be even more readable if you just embedded a new line in the string rather than using /n.

dmose · 2017-09-07T17:48:53Z

bin/render-activity-stream-html.js

@@ -44,8 +45,7 @@ function templateHTML(options, html) {
    <div id="root">${isPrerendered ? html : ""}</div>
    <div id="snippets-container">
      <div id="snippets"></div>
-    </div>${isPrerendered ? `
-    <script src="${options.baseUrl}data/content/activity-stream-initial-state.js"></script>` : ""}
+    </div>${isPrerendered ? `${scriptForPrerendering}` : ""}


Much nicer; thanks!

k88hudson · 2017-09-07T18:53:02Z

Ok, I filed a bunch of follow-ups and added them to this meta-bug: #3372

k88hudson · 2017-09-07T19:20:31Z

Filed a bugzilla ticket for the aboutNewTabService changes that need to land in mc:
https://bugzilla.mozilla.org/show_bug.cgi?id=1397875

dmose · 2017-09-07T20:26:33Z

system-addon/test/unit/activity-stream-prerender.test.jsx

+  });
+  it("should start uninitialized", () => {
+    const store = prerenderStore();
+    const state = store.getState();


In all of these tests, calling getState wants to be part of the verification block, not the execution block.

k88hudson · 2017-09-08T03:15:34Z

@Mardak @dmose

Ok, I added three more commits;

56df179 resolves the issue where a request can happen before activity stream has initialized;
c891f96 restores the class on the body element;
5614734 resolves the issue that @dmose and I talked that causes an empty locale to be rendered in the UI by waiting for LOCALE_UPDATED to be fired before replying

dmose · 2017-09-08T05:07:36Z

Do we need/want to run this through Talos again, given that the performance characteristics have probably been changed a bit by the race fixes?

Mardak · 2017-09-08T06:02:12Z

This will need a patch to keep pine / m-c happy:

diff --git a/browser/base/content/test/static/browser_all_files_referenced.js b/browser/base/content/test/static/browser_all_files_referenced.js
--- a/browser/base/content/test/static/browser_all_files_referenced.js
+++ b/browser/base/content/test/static/browser_all_files_referenced.js
@@ -88,4 +88,8 @@ var whitelist = [
   {file: "resource://app/modules/NewTabWebChannel.jsm"},
 
+  // browser/extensions/activity-stream/data/content/activity-stream-prerendered.html
+  // will be used very soon by bug 1397875
+  {file: "resource://activity-stream/data/content/activity-stream-prerendered.html"},
+
   // layout/mathml/nsMathMLChar.cpp
   {file: "resource://gre/res/fonts/mathfontSTIXGeneral.properties"},

Mardak · 2017-09-08T06:33:54Z

Looks like this test is failing with this PR:
browser/components/search/test/browser_searchEngine_behaviors.js
https://treeherder.mozilla.org/logviewer.html#?job_id=129399493&repo=try&lineNumber=3546-3549

 FAIL | browser/components/search/test/browser_searchEngine_behaviors.js | Test timed out [log…]

Some brief debugging points to it waiting for and never getting content search events:
https://searchfox.org/mozilla-central/source/browser/components/search/test/browser_searchEngine_behaviors.js#96-107

It's failing pretty much everywhere: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d54a70adfbd8b9f71db2a7062b9a9899725c70c&filter-searchStr=(bc&filter-resultStatus=testfailed&selectedJob=129397977

k88hudson · 2017-09-08T14:27:32Z

Weird ok, I'll take a look

k88hudson · 2017-09-08T15:55:10Z

@Mardak @dmose ok, 5d84448 fixes the race condition where Search was getting loaded to early for the event to fire, as well as the unreferenced file.

dmose · 2017-09-08T16:55:36Z

mozilla-central-patches/fix-search-test-race.diff

+-        }
+-      });
+-    });
+    await ContentTaskUtils.waitForCondition(() => content.wrappedJSObject.gContentSearchController &&


It'd be more readable if each of the "content.wrappedJSObject" clauses had its own line, eslint-permitting.

The original style of test seems more robust, though I get that it wasn't quite right. This fix seems ok for now, but I'd suggest also adding an XXX comment suggesting adding an event to the code or some such so that the test depends less on implementation details and is more useful for refactoring.

I don't really see how this relies more on implementation details, given that that particular event has to be fired (and also in a particular order, i.e. after the listener is added in the test). Additionally, I kind of feel like using the event makes this test way harder to debug/step through 😜

OK, that's a reasonable enough rationale.

dmose · 2017-09-08T17:55:32Z

The aboutNewTabService patch to firefox.js seems to have bit-rotted. Re-create it, using -U2 so that it will be more robust and likely to last a bit longer?

This prevents the checksum from failing for prerendering due to the state in the initial render being different than what it expects.

Mardak · 2017-09-08T18:37:55Z

There's a similar browser_google_behavior that has a "promiseContentSearchReady" but isn't failing ?
https://searchfox.org/mozilla-central/search?q=promiseContentSearchReady

dmose

r=dmose

as-pine-proxy · 2017-09-08T19:19:05Z

Treeherder: 524b2f921428edc800ec2e33de4d85caf8719911

Mardak · 2017-09-12T13:10:04Z

I reverted with ade8033 part of 84534e4 that added mozilla-central-patches/fix-search-test-race.diff now that the export is on m-c with https://hg.mozilla.org/mozilla-central/rev/57f1221bf914

k88hudson force-pushed the k88hudson:gh2813 branch from 686518a to 09f43dd Sep 6, 2017

dmose reviewed Sep 6, 2017

View changes

system-addon/common/PerfService.jsm Outdated

usablePerfObj = {

now() {},

mark() {}

};

This comment has been minimized.

Sign in to view

dmose Sep 6, 2017
Member

I'm assuming this is needed so that it runs under node. A comment indicating that would nice.

dmose reviewed Sep 6, 2017

View changes

Mardak reviewed Sep 6, 2017

View changes

Mardak reviewed Sep 7, 2017

View changes

system-addon/data/content/activity-stream.html

<body class="activity-stream">

<div id="root"></div>

<div id="snippets-container">

<div id="snippets"></div>

This comment has been minimized.

Sign in to view

Mardak Sep 7, 2017
Member

We'll still need these snippets related div#ids, yeah?

k88hudson assigned dmose Sep 7, 2017

dmose reviewed Sep 7, 2017

View changes

bin/render-activity-stream-html.js Outdated

@@ -1,3 +1,4 @@

/* eslint-disable no-sync */ /* eslint-disable no-sync */ /* eslint-disable no-sync */

This comment has been minimized.

Sign in to view

dmose Sep 7, 2017
Member

Three times? :-)

dmose reviewed Sep 7, 2017

View changes

k88hudson force-pushed the k88hudson:gh2813 branch from 8ae59ca to 56df179 Sep 8, 2017

dmose reviewed Sep 8, 2017

View changes

k88hudson added 10 commits Sep 5, 2017

feat(mc): #2813 Refactor various files to account for prerendering

7bf92b9

feat(mc): #2813 Send first render info on NEW_TAB_STATE_REQUEST

1b0fbb2

feat(mc): #2813 Add buildmc:html task, prerendering

5e613f4

feat(mc): #2813 Allow topics to be rendered but empty

bf2540d

feat(mc): #2813 Add mc patch for URL switching in aboutNewTabService.js

90a4160

feat(mc): #2813 initStore should discard msgs before MERGE_STORE_ACTION

a82b82b

This prevents the checksum from failing for prerendering due to the state in the initial render being different than what it expects.

feat(mc): #2813 Re-request rehydration if INIT was received after

210d2a9

fix(mc): #2813 Add body class back in

ec1b3d7

fix(mc): #3366 Wait for strings before replying in NewTabInit

3b5e37b

test(mc): #2813 Fix related failing mc tests

84534e4

k88hudson force-pushed the k88hudson:gh2813 branch from 5d84448 to 84534e4 Sep 8, 2017

dmose approved these changes Sep 8, 2017

View changes

k88hudson merged commit 5779a88 into mozilla:master Sep 8, 2017
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

as-pine-proxy added the pushed-to-pine label Sep 8, 2017

This was referenced Sep 8, 2017

Turn on prerendered version of activity stream #3396

Closed

Perma- FAIL browser_google_behavior.js | Test timed out #3397

Closed

Mardak mentioned this pull request Sep 12, 2017

Add tests for NewTabInit.jsm and wait for strings to exist #3366

Closed

		@@ -49,20 +49,23 @@ const messageMiddleware = store => next => action => {
		* @param {object} reducers An object containing Redux reducers

		@@ -1,3 +1,4 @@
		/* eslint-disable no-sync / / eslint-disable no-sync / / eslint-disable no-sync */

		@@ -82,5 +82,5 @@ class Search extends React.Component {
		}

		// initialized is passed to props so that Search will rerender when it receives strings

mozilla / activity-stream Archived

Prerendering #3359

Prerendering #3359

Conversation

k88hudson commented Sep 6, 2017 • edited

This comment has been minimized.

dmose Sep 6, 2017 Member

This comment has been minimized.

dmose Sep 6, 2017 Member

This comment has been minimized.

k88hudson Sep 6, 2017 Author Member

This comment has been minimized.

dmose Sep 6, 2017 Member

This comment has been minimized.

dmose Sep 6, 2017 Member

This comment has been minimized.

dmose Sep 6, 2017 Member

This comment has been minimized.

k88hudson Sep 7, 2017 Author Member

This comment has been minimized.

dmose Sep 7, 2017 Member

This comment has been minimized.

dmose Sep 6, 2017 Member

This comment has been minimized.

dmose Sep 6, 2017 Member

This comment has been minimized.

dmose Sep 6, 2017 Member

This comment has been minimized.

Mardak Sep 6, 2017 • edited Member

This comment has been minimized.

Mardak Sep 6, 2017 Member

Mardak commented Sep 7, 2017

This comment has been minimized.

Mardak Sep 7, 2017 Member

Mardak commented Sep 7, 2017

This comment has been minimized.

dmose Sep 7, 2017 Member

This comment has been minimized.

dmose Sep 7, 2017 Member

This comment has been minimized.

dmose Sep 7, 2017 Member

This comment has been minimized.

dmose Sep 7, 2017 Member

This comment has been minimized.

dmose Sep 7, 2017 Member

This comment has been minimized.

dmose Sep 7, 2017 Member

This comment has been minimized.

k88hudson Sep 7, 2017 Author Member

This comment has been minimized.

dmose Sep 7, 2017 Member

This comment has been minimized.

dmose Sep 7, 2017 Member

This comment has been minimized.

k88hudson Sep 7, 2017 Author Member

This comment has been minimized.

k88hudson Sep 7, 2017 Author Member

This comment has been minimized.

dmose Sep 7, 2017 Member

This comment has been minimized.

dmose Sep 7, 2017 Member

This comment has been minimized.

k88hudson Sep 7, 2017 Author Member

This comment has been minimized.

dmose Sep 7, 2017 Member

This comment has been minimized.

dmose Sep 7, 2017 Member

This comment has been minimized.

dmose Sep 7, 2017 Member

This comment has been minimized.

dmose Sep 7, 2017 Member

This comment has been minimized.

dmose Sep 7, 2017 Member

This comment has been minimized.

dmose Sep 7, 2017 Member

This comment has been minimized.

dmose Sep 7, 2017 Member

This comment has been minimized.

dmose Sep 7, 2017 Member

k88hudson commented Sep 7, 2017

k88hudson commented Sep 6, 2017 •

edited

dmose Sep 6, 2017
Member

dmose Sep 6, 2017
Member

k88hudson Sep 6, 2017
Author Member

dmose Sep 6, 2017
Member

dmose Sep 6, 2017
Member

dmose Sep 6, 2017
Member

k88hudson Sep 7, 2017
Author Member

dmose Sep 7, 2017
Member

dmose Sep 6, 2017
Member

dmose Sep 6, 2017
Member

dmose Sep 6, 2017
Member

Mardak Sep 6, 2017 •

edited

Member

Mardak Sep 6, 2017
Member

Mardak Sep 7, 2017
Member

dmose Sep 7, 2017
Member

dmose Sep 7, 2017
Member

dmose Sep 7, 2017
Member

dmose Sep 7, 2017
Member

dmose Sep 7, 2017
Member

dmose Sep 7, 2017
Member

k88hudson Sep 7, 2017
Author Member

dmose Sep 7, 2017
Member

dmose Sep 7, 2017
Member

k88hudson Sep 7, 2017
Author Member

k88hudson Sep 7, 2017
Author Member

dmose Sep 7, 2017
Member

dmose Sep 7, 2017
Member

k88hudson Sep 7, 2017
Author Member

dmose Sep 7, 2017
Member

dmose Sep 7, 2017
Member

dmose Sep 7, 2017
Member

dmose Sep 7, 2017
Member

dmose Sep 7, 2017
Member

dmose Sep 7, 2017
Member

dmose Sep 7, 2017
Member

dmose Sep 7, 2017
Member

dmose Sep 7, 2017
Member

k88hudson commented Sep 8, 2017 •

edited

Mardak commented Sep 8, 2017 •

edited

k88hudson commented Sep 8, 2017 •

edited

dmose Sep 8, 2017
Member

dmose Sep 8, 2017
Member

k88hudson Sep 8, 2017
Author Member

dmose Sep 8, 2017
Member

dmose commented Sep 8, 2017 •

edited

Mardak commented Sep 12, 2017 •

edited