-
Notifications
You must be signed in to change notification settings - Fork 160
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?
Conversation
9956dfa
to
991beca
Compare
991beca
to
041bae7
Compare
041bae7
to
63abd81
Compare
fastboot: this.fastboot | ||
fastboot: this.fastboot, | ||
visitOptions: { | ||
renderMode: appConfig.fastboot.renderMode |
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 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? 😂
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 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
.
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 🙈
@@ -0,0 +1,42 @@ | |||
'use strict'; |
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.
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.
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.
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 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.
@xg-wang regarding our discussion of the API of this change, here is me consuming the fastboot lib in prember: ef4/prember#66 which might be useful to see what the API looks like in the wild. Let me know what you think, and we can probably discuss it in the weekly meeting too 👍 |
This is something that we discussed in the weekly Fastboot meeting where I thought it would be useful to be able to turn on rehydration based on a config entry rather than using the environment variable.
This would allow you to create a PR with the change to turn on rehydration quite easily, and also would support most preview apps setups when using heroku or something like netlify if you're using prember
I have managed to get my little test passing, and if all the other tests pass then I'm pretty confident that this would be ok to merge but I guess that we should probably still discuss it at one of the Fastboot meetings before we do.
One interesting fact: this doesn't actually enable rehydration for prember 🙈 the issue is that prember is actually instantiating its own Fastboot instance so I would need to make a change in prember to enable passing the config to either the constructor of the instance or the visit function call 🙃