-
Notifications
You must be signed in to change notification settings - Fork 159
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
base: master
Are you sure you want to change the base?
Changes from all commits
30ffaeb
76f1a55
37ff337
779e888
0293489
fc7e105
63abd81
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
'use strict'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 😂 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can move the fixture app to |
||
|
||
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; | ||
}; |
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.
Suggestion: check input and throw a silent-error if not allowed values (
"serialize"
)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.
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 bettermaybe 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? 😂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.
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 isserialize
on server side, andrehydrate
on client side. If rehydration is not enabled, ember will useundefined
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 inenvironement.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 withenableRehydration
.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.
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.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 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 🙈