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

Turn on rehydration based on config #818

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 6 additions & 1 deletion packages/ember-cli-fastboot/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -336,8 +336,13 @@ module.exports = {
this.fastboot = new FastBoot(fastbootOptions);
}

let appConfig = this._getHostAppConfig();

let fastbootMiddleware = FastBootExpressMiddleware({
fastboot: this.fastboot
fastboot: this.fastboot,
visitOptions: {
renderMode: appConfig.fastboot.renderMode
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: check input and throw a silent-error if not allowed values ("serialize")

Copy link
Member Author

Choose a reason for hiding this comment

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

yes that's probably a good idea, but to be honest I'm not even sure if renderMode is the right thing to be passing around 🤔 right now it can only have exactly one value and that is being used to turn a feature on... which makes me feel like a boolean would be better

maybe something quite explicit like turnOnRehydration or something but I don't know 🤷 I guess the question becomes: how likely is it that we will have more render modes? 😂

Copy link
Member

@xg-wang xg-wang Apr 4, 2021

Choose a reason for hiding this comment

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

Yea I agree with you here renderMode is not the right public interface we expose.

renderMode is what ember-cli-fastboot passes to ember: when rehydration is enabled, it is serialize on server side, and rehydrate on client side. If rehydration is not enabled, ember will use undefined renderMode to use the default clientBuilder (see https://github.com/emberjs/ember.js/blob/4cb7da45fd6e4720daf7e769877b3d8868a3cf71/packages/%40ember/-internals/glimmer/lib/setup-registry.ts#L25-L38)

User should not be aware of these wirings, let's call it enableRehydration which people can set in environement.fastboot.enableRehydration.

The config will be picked up by your change here and set renderMode which ember/glimmer understand.

I'd like to deprecate the EXPERIMENTAL_RENDER_MODE_SERIALIZE with the PR to use the config flag instead.

For Prember if fastboot is upgraded to latest, rehydration will be enabled if the app is configured with enableRehydration.

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively we can set it in fastboot.visit(path, { enableRehydration }), then per visit is allowed have different behavior. But I don't think it is necessary since user will have a single flag to control all visit behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

I already have a corresponding PR on Prember that I know works if you wanted to check it out ef4/prember#66

I totally agree with your suggestion for the public API and I'll get that done when I get a chance 👍

I hadn't actually thought about deprecating the environment variable 🤔 I guess it's a good idea so I'll add that to this PR too

While I agree that in my use case and in most use cases we would probably enable rehydration holistically for the whole app at once, @rwjblue did specifically ask that we expose a per-request API for people that didn't want to turn it on for everything or wanted to experiment with A/B tests for example. I have no preference on this particular point because I'm very much going to be enabling it for a full Prember run all at once so I can't really defend this position well since it's not my idea 🙈

}
});

fastbootMiddleware(req, resp, next);
Expand Down
5 changes: 5 additions & 0 deletions packages/ember-cli-fastboot/lib/broccoli/fastboot-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@ module.exports = class FastBootConfig extends Plugin {
this.fastbootConfig[appConfigModule] = options.appConfig;
this._fileToChecksumMap = {};

if (options.appConfig.fastboot) {
this.renderMode = options.appConfig.fastboot.renderMode;
}

if (this.fastbootAppConfig && this.fastbootAppConfig.htmlFile) {
this.htmlFile = this.fastbootAppConfig.htmlFile;
} else {
Expand Down Expand Up @@ -174,6 +178,7 @@ module.exports = class FastBootConfig extends Plugin {
schemaVersion: LATEST_SCHEMA_VERSION,
manifest: this.manifest,
hostWhitelist: this.normalizeHostWhitelist(),
renderMode: this.renderMode,
config: this.fastbootConfig,
appName: this.appName,
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
'use strict';
Copy link
Member

Choose a reason for hiding this comment

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

We can start writing tests involving fastboot / ember-cli-fastboot in https://github.com/ember-fastboot/ember-cli-fastboot/blob/2a6ed5d76b00da2106a9ba10f048aa2bef989a80/test-packages/integration-tests/test/basic-test.js.
But we can land this PR and do another follow up to move later.

Copy link
Member Author

Choose a reason for hiding this comment

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

oh is this the new place that @kiwiupover has been working on? to be honest I wasn't sure and I just copy and pasted an existing test to get it to work. I would much prefer to do it the right way in this PR rather than fix it up in another one, I just don't exactly know how 😂

Copy link
Member

Choose a reason for hiding this comment

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

You can move the fixture app to test-packages, then assert result in test-packages/integration-tests/test/basic-test.js. But don't worry I can get a test when we figure out how we want to name the config flag.


const expect = require('chai').use(require('chai-string')).expect;
const RSVP = require('rsvp');
const request = RSVP.denodeify(require('request'));

const AddonTestApp = require('ember-cli-addon-tests').AddonTestApp;

describe('FastBoot renderMode config', function() {
this.timeout(400000);

let app;

before(function() {
app = new AddonTestApp();

return app.create('fastboot-rendermode-config', { emberVersion: 'latest'})
.then(function() {
return app.startServer({
command: 'serve'
});
});
});

after(function() {
return app.stopServer();
});

it('uses rehydration when rendermode is serialize', function() {
return request({
url: 'http://localhost:49741/dynamic',
headers: {
'Accept': 'text/html'
}
})
.then(function(response) {
expect(response.statusCode).to.equal(200);
expect(response.headers['content-type']).to.equalIgnoreCase('text/html; charset=utf-8');
expect(response.body).to.contain('<!--%+b:7%-->magic<!--%-b:7%-->');
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import Ember from 'ember';

let Router = Ember.Router;

Router.map(function() {
this.route('dynamic');
});

export default Router;
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import Ember from 'ember';

export default Ember.Route.extend({
model() {
return 'magic';
}
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
<h1>This should be a dynamic page!</h1>

<p>And the most dynamic word is: {{@model}}</p>
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
'use strict';

module.exports = function(environment) {
var ENV = {
rootURL: '/',
locationType: 'auto',
environment: environment,
modulePrefix: 'fastboot-rendermode-config',
fastboot: {
hostWhitelist: [/localhost:\d+/],
renderMode: 'serialize',
}
};

return ENV;
};
11 changes: 7 additions & 4 deletions packages/fastboot/src/ember-app.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ class EmberApp {
this.appName = config.appName;
this.html = config.html;
this.sandboxRequire = config.sandboxRequire;
this.renderMode = config.renderMode;

if (process.env.APP_CONFIG) {
let appConfig = JSON.parse(process.env.APP_CONFIG);
Expand Down Expand Up @@ -298,7 +299,10 @@ class EmberApp {
let buildSandboxPerVisit = options.buildSandboxPerVisit || false;

let shouldRender = options.shouldRender !== undefined ? options.shouldRender : true;
let bootOptions = buildBootOptions(shouldRender);
let renderMode = process.env.EXPERIMENTAL_RENDER_MODE_SERIALIZE
? 'serialize'
: options.renderMode || this.renderMode;
let bootOptions = buildBootOptions(shouldRender, renderMode);
let fastbootInfo = new FastBootInfo(req, res, {
hostWhitelist: this.hostWhitelist,
metadata: options.metadata,
Expand Down Expand Up @@ -355,17 +359,16 @@ class EmberApp {
* Builds an object with the options required to boot an ApplicationInstance in
* FastBoot mode.
*/
function buildBootOptions(shouldRender) {
function buildBootOptions(shouldRender, renderMode) {
let doc = new SimpleDOM.Document();
let rootElement = doc.body;
let _renderMode = process.env.EXPERIMENTAL_RENDER_MODE_SERIALIZE ? 'serialize' : undefined;

return {
isBrowser: false,
document: doc,
rootElement,
shouldRender,
_renderMode,
_renderMode: renderMode,
};
}

Expand Down
1 change: 1 addition & 0 deletions packages/fastboot/src/fastboot-schema.js
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ function loadConfig(distPath) {
scripts,
html,
hostWhitelist: pkg.fastboot.hostWhitelist,
renderMode: pkg.fastboot.renderMode,
config,
appName,
sandboxRequire,
Expand Down