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

Utilize rehydration serialization from glimmer #580

Conversation

rondale-sc
Copy link
Contributor

@rondale-sc rondale-sc commented Feb 23, 2018

This is part of an arching plan to introduce Glimmer's
rehydration/serializtion modes to Ember proper.

There are 4 PR's that are all interwoven, of which, this is one.

In order of need to land they are:

Glimmer.js:
glimmerjs/glimmer-vm#783 (comment)

This resolves a rather intimate API problem. Glimmer-vm expects a very
specific comment node to exist to know whether or not the rehydration
element builder can do it's job properly. If it is not found on the
first node from rootElement it throws.

In fastboot however we are certain that there will already be existant
elements in the way that will happen before rendered content.

This PR just iterates through the nodes until it finds the expected
comment node. And only throws if it never finds one.


Ember.js:
emberjs/ember.js#16227

This PR modifies the visit API to allow a private _renderMode to be
set to either serialize or rehydrate. In SSR environments the
assumption (as it is here in this fastboot PR) is that we'll set the
visit API to serialize mode which ensures glimmer-vm's serialize element
builder is used to build the API.

The serialize element builder ensures that we have the necessary fidelty
to rehydrate correctly and is mandatory input for rehydration.


Fastboot:
ember-fastboot/fastboot#185

This allows enviroment variable to set _renderMode to be used in Visit
API. Fastboot must send content to browser made with the serialization
element builder to ensure rehydration can be sucessful.


EmberCLI Fastboot:
#580

Finally this does the fun part of disabling the current
clear-double-render instance-initializer

We first check to ensure we are in a non-fastboot environment. Then we
ensure that we can find the expected comment node from glimmer-vm's
serialize element builder. This ensures that this change will only
effect peoeple who use ember-cli-fastboot with the serialized output
from the currently experimental fastboot setup

Then we ensure ApplicationInstance#_bootSync specifies the rehydrate
_renderMode.

This is done in _bootSync this way because visit is currently not used
to boot ember applications. And we must instead set bootOptions this
way instead.

We also remove the markings for fastboot-body-start and
fastboot-body-end to ensure clear-double render instance-initializer
is never called.

rondale-sc added a commit to rondale-sc/fastboot that referenced this pull request Feb 23, 2018
This is part of an arching plan to introduce Glimmer's
rehydration/serializtion modes to Ember proper.

There are 4 PR's that are all interwoven, of which, this is one.

In order of need to land they are:

Glimmer.js:
glimmerjs/glimmer-vm#783 (comment)

This resolves a rather intimate API problem.  Glimmer-vm expects a very
specific comment node to exist to know whether or not the rehydration
element builder can do it's job properly.  If it is not found on the
first node from `rootElement` it throws.

In fastboot however we are certain that there will already be existant
elements in the way that will happen before rendered content.

This PR just iterates through the nodes until it finds the expected
comment node.  And only throws if it never finds one.

---

Ember.js:
emberjs/ember.js#16227

This PR modifies the `visit` API to allow a private _renderMode to be
set to either serialize or rehydrate.  In SSR environments the
assumption (as it is here in this fastboot PR) is that we'll set the
visit API to serialize mode which ensures glimmer-vm's serialize element
builder is used to build the API.

The serialize element builder ensures that we have the necessary fidelty
to rehydrate correctly and is mandatory input for rehydration.

---

Fastboot:
ember-fastboot#185

This allows enviroment variable to set _renderMode to be used in Visit
API.  Fastboot must send content to browser made with the serialization
element builder to ensure rehydration can be sucessful.

---

EmberCLI Fastboot:
ember-fastboot/ember-cli-fastboot#580

Finally this does the fun part of disabling the current
clear-double-render instance-initializer

We first check to ensure we are in a non-fastboot environment.  Then we
ensure that we can find the expected comment node from glimmer-vm's
serialize element builder.  This ensures that this change will only
effect peoeple who use ember-cli-fastboot with the serialized output
from the currently experimental fastboot setup

Then we ensure `ApplicationInstance#_bootSync` specifies the rehydrate
_renderMode.

This is done in `_bootSync` this way because visit is currently not used
to boot ember applications.  And we must instead set bootOptions this
way instead.

We also remove the markings for `fastboot-body-start` and
`fastboot-body-end` to ensure clear-double render instance-initializer
is never called.
@rondale-sc rondale-sc force-pushed the utilize-rehydration-serialization-from-glimmer branch from 91483a1 to 37d1f21 Compare February 23, 2018 20:53
rondale-sc added a commit to rondale-sc/ember-cli-fastboot that referenced this pull request Feb 23, 2018
This is part of an arching plan to introduce Glimmer's
rehydration/serializtion modes to Ember proper.

There are 4 PR's that are all interwoven, of which, this is one.

- [ ] Glimmer.js: glimmerjs/glimmer-vm#783 (comment)
- [ ] Ember.js: emberjs/ember.js#16227
- [ ] Fastboot: ember-fastboot/fastboot#185
- [ ] EmberCLI Fastboot: ember-fastboot#580

In order of need to land they are:

Glimmer.js:
glimmerjs/glimmer-vm#783 (comment)

This resolves a rather intimate API problem.  Glimmer-vm expects a very
specific comment node to exist to know whether or not the rehydration
element builder can do it's job properly.  If it is not found on the
first node from `rootElement` it throws.

In fastboot however we are certain that there will already be existant
elements in the way that will happen before rendered content.

This PR just iterates through the nodes until it finds the expected
comment node.  And only throws if it never finds one.

---

Ember.js:
emberjs/ember.js#16227

This PR modifies the `visit` API to allow a private _renderMode to be
set to either serialize or rehydrate.  In SSR environments the
assumption (as it is here in this fastboot PR) is that we'll set the
visit API to serialize mode which ensures glimmer-vm's serialize element
builder is used to build the API.

The serialize element builder ensures that we have the necessary fidelty
to rehydrate correctly and is mandatory input for rehydration.

---

Fastboot:
ember-fastboot/fastboot#185

This allows enviroment variable to set _renderMode to be used in Visit
API.  Fastboot must send content to browser made with the serialization
element builder to ensure rehydration can be sucessful.

---

EmberCLI Fastboot:
ember-fastboot#580

Finally this does the fun part of disabling the current
clear-double-render instance-initializer

We first check to ensure we are in a non-fastboot environment.  Then we
ensure that we can find the expected comment node from glimmer-vm's
serialize element builder.  This ensures that this change will only
effect peoeple who use ember-cli-fastboot with the serialized output
from the currently experimental fastboot setup

Then we ensure `ApplicationInstance#_bootSync` specifies the rehydrate
_renderMode.

This is done in `_bootSync` this way because visit is currently not used
to boot ember applications.  And we must instead set bootOptions this
way instead.

We also remove the markings for `fastboot-body-start` and
`fastboot-body-end` to ensure clear-double render instance-initializer
is never called.
var current = document.getElementById('fastboot-body-start');

if (current && current.nextSibling.nodeValue === '%+b:0%') {
Ember.ApplicationInstance.reopen({
Copy link
Member

Choose a reason for hiding this comment

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

Is this how we plan to do this long term? Do we have a plan to remove this reopen?

package.json Outdated
@@ -29,7 +29,7 @@
"ember-cli-lodash-subset": "2.0.1",
"ember-cli-preprocess-registry": "^3.1.0",
"ember-cli-version-checker": "^2.1.0",
"fastboot": "^1.1.3",
"fastboot": "github:rondale-sc/fastboot#utilize-rehydration-serialization-from-glimmer",
Copy link
Member

Choose a reason for hiding this comment

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

Just notating that this will need to be bumped to a release version before landing...

if (typeof FastBoot === 'undefined') {
var current = document.getElementById('fastboot-body-start');

if (current && current.nextSibling.nodeValue === '%+b:0%') {
Copy link
Member

Choose a reason for hiding this comment

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

We really really need a way to avoid encoding this, can you make an issue in glimmer-vm to expose a isRehydrationComment function that both Ember and ember-cli-fastboot can use? I'm concerned with the serialize markers changing over time (they have already changed once!?!) and then ember-cli-fastboot will have issues with cross-ember compat...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I totally agree! I'll make the issue.

rondale-sc added a commit to rondale-sc/ember.js that referenced this pull request Feb 27, 2018
This is dependent on this glimmer-vm PR to actually work glimmerjs/glimmer-vm#788. Which means we'll need to wait until it is merged/bumped and Ember itself updates to use the release with it in it before we can land this.

The idea here is to insulate things that depend on the magic string we use to determine whether or not the DOM node we pass was produced by the serialization element builder.

This needs to be exposed on the global to ensure we can use it in the ember-cli-fastboot vendor file so we can determine how to boot the app when it is loaded in the browser.

To see where it'd be used in ember-cli-fastboot please see this PR with the in progress work:

ember-fastboot/ember-cli-fastboot#580
rondale-sc added a commit to rondale-sc/ember.js that referenced this pull request Mar 6, 2018
This is dependent on this glimmer-vm PR to actually work glimmerjs/glimmer-vm#788. Which means we'll need to wait until it is merged/bumped and Ember itself updates to use the release with it in it before we can land this.

The idea here is to insulate things that depend on the magic string we use to determine whether or not the DOM node we pass was produced by the serialization element builder.

This needs to be exposed on the global to ensure we can use it in the ember-cli-fastboot vendor file so we can determine how to boot the app when it is loaded in the browser.

To see where it'd be used in ember-cli-fastboot please see this PR with the in progress work:

ember-fastboot/ember-cli-fastboot#580
@rondale-sc rondale-sc closed this Mar 6, 2018
@rondale-sc rondale-sc reopened this Mar 6, 2018
@rondale-sc
Copy link
Contributor Author

Closed and reopened to trigger another Travis build

@rondale-sc
Copy link
Contributor Author

@rwjblue This is ready once ember-fastboot/fastboot#185 lands

@rondale-sc rondale-sc force-pushed the utilize-rehydration-serialization-from-glimmer branch from 7d36f20 to a791e17 Compare March 7, 2018 16:46
@rondale-sc rondale-sc closed this Mar 9, 2018
@rondale-sc rondale-sc reopened this Mar 9, 2018
@rondale-sc
Copy link
Contributor Author

@kratiahuja @rwjblue I'm not sure why these tests are failing. They pass locally, and the only difference seems to be capitalization of the Content Type Header in the acceptance tests

Copy link
Contributor

@kratiahuja kratiahuja left a comment

Choose a reason for hiding this comment

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

How would apps opt in into the process.env.EXPERIMENTAL_RENDER_MODE_SERIALIZE ?

var current = document.getElementById('fastboot-body-start');

if (current && Ember.ViewUtils.isSerializationFirstNode(current.nextSibling)) {
Ember.ApplicationInstance.reopen({
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The clear-double-boot checks to see if fastboot-body-start is present. If we detect true with Ember.ViewUtils.isSerializationFirstNode we eventually delete fastboot-body-start which prevents the clear-double-render.

if (typeof FastBoot === 'undefined') {
var current = document.getElementById('fastboot-body-start');

if (current && Ember.ViewUtils.isSerializationFirstNode(current.nextSibling)) {
Copy link
Contributor

@kratiahuja kratiahuja Mar 10, 2018

Choose a reason for hiding this comment

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

Why not provide a mixin for folks to add in their app.js itself? I am not sure I am sold on the idea of reopening.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So long as Ember.ViewUtils.isSerializationFirstNode is present consumers of fastboot will never have to worry about this line of code unless fastboot actually encodes the DOM the browser version of Ember receives with the glimmer-vm's serialization element builder. And if the the isSerializationFirstNode triggers true then we know the user has opted in.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the Ember.ViewUtils.isSerializationFirstNode a public API?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kratiahuja It is public.

Copy link
Member

Choose a reason for hiding this comment

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

@rondale-sc - Can you tweak this to be safe when Ember.ViewUtils.isSerializationFirstNode is not present (e.g. you aren’t using >= some 3.2ish canary build ATM)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rwjblue Updated to ensure typeof Ember.ViewUtils.isSerializationFirstNode === 'function'

Copy link
Member

Choose a reason for hiding this comment

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

Thank you!

@kratiahuja
Copy link
Contributor

General question: How will apps opt in or opt out of rehydration? If there are issues, I would rather not have apps break and are forced to not use fastboot.

@rondale-sc
Copy link
Contributor Author

@kratiahuja The way to opt-in once this PR is merged would be to ensure the process.env.EXPERIMENTAL_RENDER_MODE_SERIALIZE is set to true. If that happens all the rest of the pipeline for rehydration should just work based off of that.

@kratiahuja
Copy link
Contributor

We discussed this also needs doc updates :)

This is part of an arching plan to introduce Glimmer's
rehydration/serializtion modes to Ember proper.

There are 4 PR's that are all interwoven, of which, this is one.

- [ ] Glimmer.js: glimmerjs/glimmer-vm#783 (comment)
- [ ] Ember.js: emberjs/ember.js#16227
- [ ] Fastboot: ember-fastboot/fastboot#185
- [ ] EmberCLI Fastboot: ember-fastboot#580

In order of need to land they are:

Glimmer.js:
glimmerjs/glimmer-vm#783 (comment)

This resolves a rather intimate API problem.  Glimmer-vm expects a very
specific comment node to exist to know whether or not the rehydration
element builder can do it's job properly.  If it is not found on the
first node from `rootElement` it throws.

In fastboot however we are certain that there will already be existant
elements in the way that will happen before rendered content.

This PR just iterates through the nodes until it finds the expected
comment node.  And only throws if it never finds one.

---

Ember.js:
emberjs/ember.js#16227

This PR modifies the `visit` API to allow a private _renderMode to be
set to either serialize or rehydrate.  In SSR environments the
assumption (as it is here in this fastboot PR) is that we'll set the
visit API to serialize mode which ensures glimmer-vm's serialize element
builder is used to build the API.

The serialize element builder ensures that we have the necessary fidelty
to rehydrate correctly and is mandatory input for rehydration.

---

Fastboot:
ember-fastboot/fastboot#185

This allows enviroment variable to set _renderMode to be used in Visit
API.  Fastboot must send content to browser made with the serialization
element builder to ensure rehydration can be sucessful.

---

EmberCLI Fastboot:
ember-fastboot#580

Finally this does the fun part of disabling the current
clear-double-render instance-initializer

We first check to ensure we are in a non-fastboot environment.  Then we
ensure that we can find the expected comment node from glimmer-vm's
serialize element builder.  This ensures that this change will only
effect peoeple who use ember-cli-fastboot with the serialized output
from the currently experimental fastboot setup

Then we ensure `ApplicationInstance#_bootSync` specifies the rehydrate
_renderMode.

This is done in `_bootSync` this way because visit is currently not used
to boot ember applications.  And we must instead set bootOptions this
way instead.

We also remove the markings for `fastboot-body-start` and
`fastboot-body-end` to ensure clear-double render instance-initializer
is never called.
This is dependent on glimmerjs/glimmer-vm#788

The idea is to use a function exported by ember-glimmer rather than rely
on magic glimmer-vm string.
@rondale-sc rondale-sc force-pushed the utilize-rehydration-serialization-from-glimmer branch from a791e17 to b3d2589 Compare March 10, 2018 20:05

https://github.com/glimmerjs/glimmer-vm/commit/316805b9175e01698120b9566ec51c88d075026a

In order to utilize rehydration in Ember.js applications we need to ensure that
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't you need to mention from which version it is available?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely! I just added a small blurb about fastboot / ember.js requirements to address this concern.

@kratiahuja
Copy link
Contributor

[email protected] is released.

README.md Outdated
by the serialization builder. If it was, it tells the Ember.js Application to
use the rehydration builder and your application will be using rehydration.

Rehydration is only compatible with fastboot > 1.1.4, and Ember.js > 3.2.
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be 1.1.4-beta.1

Copy link
Contributor

@kratiahuja kratiahuja left a comment

Choose a reason for hiding this comment

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

Once the below is updated and tests pass, this is good to land. We need more tests around this to make sure this doesn't regress but we can do it in the next beta release.

package.json Outdated
@@ -29,7 +29,7 @@
"ember-cli-lodash-subset": "2.0.1",
"ember-cli-preprocess-registry": "^3.1.0",
"ember-cli-version-checker": "^2.1.0",
"fastboot": "^1.1.3",
"fastboot": "github:rondale-sc/fastboot#utilize-rehydration-serialization-from-glimmer",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change this to 1.1.4-beta.1?

@bantic
Copy link
Contributor

bantic commented Mar 13, 2018

This needs an update to the yarn.lock also — I'll push that in a moment

@bantic
Copy link
Contributor

bantic commented Mar 13, 2018

Pushed the updated yarn.lock. @kratiahuja This should be good to go now as soon as tests pass

@kratiahuja
Copy link
Contributor

kratiahuja commented Mar 13, 2018

thank you @bantic . I'll merge to master once tests pass and cut a release for beta. I will also create a branch for stable if required and keep that seperate.

@rondale-sc
Copy link
Contributor Author

Thanks @bantic! GitHub editing on an iPad can only get me so far 😜.

@kratiahuja
Copy link
Contributor

Thank you @rondale-sc and @bantic ! Merging this and releasing.

@kratiahuja kratiahuja merged commit 1eeef40 into ember-fastboot:master Mar 13, 2018
@kratiahuja
Copy link
Contributor

[email protected] is published.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants