Use `-in-element` to render at appropriate time #37

rondale-sc · 2017-08-03T19:02:02Z

This PR attempts to address the fact that in ember-cli-head's initializer we call component.appendTo(document.head). This triggers a rendering cycle where Glimmer fully processes the head-layout component and then appends it to the document head before the application's main rendering process.

We decided not to do this:

~~If we don't want to do a major version bump we need to do the following:~~

~~move head-browser-initializer to the addon folder~~
- ~~Use ember-compatibility-helpers to remove the component.appendTo(document.head) only when GLIMMER_2 is found~~
~~Use the treeForFastboot hook to emit the old fastboot files when Ember version of application is < 2.10~~
~~Use an AST transform to modify the user's application.hbs to include the invocation of {{head-layout}} component.~~
- ~~In an ideal world we'd be able to do this more naively in a broccoli-plugin where we just find the actual template file and add it there, but unsure how/if we could do that.~~

Details

Currently it appends before the didTransition hook from the router happens, which means it is doing this work in the wrong phase of execution. I did some benchmarking with ember-macro-benchmark and noticed this change when using this PR. The experiment is our PR here.

Which shows that we are moving the work to the correct place. We can observe this more clearly in the Chrome timeline.

You can see in the above that ember-cli-head triggers a render before the main rendering pass for our application's first frame.

With this PR it looks more like this:

That's the TL;DR (which is too long)

But this is still very much WIP. After discussing this with @rwjblue we decided that this would necessitate a major version bump unless we did a few things to make it backward compat.

Benefits

I'd like to start a discussion about the possible benefits here. With my benchmarking tools I couldn't see a pronounced difference (it isn't 100% clear yet). Though, I would imagine there is some duplicated effort in starting the rendering process from a cold-start twice.

Any thoughts or ideas would be very much appreciated.

kratiahuja

Overall I am +1 to this approach. It also makes sense to do a major version bump.

kratiahuja · 2017-08-04T19:54:19Z

package.json

@@ -26,6 +26,7 @@
    "ember-cli-app-version": "^1.0.0",
    "ember-cli-dependency-checker": "^1.3.0",
    "ember-cli-eslint": "^3.0.0",
+    "ember-cli-fastboot": "^1.0.2",


Why this is added as a devDependnecy?

Primarily to ensure this approach worked in fastboot. Means that we can run ember s and disable JS then inspect the head element. I can remove if that is a problem

Definitely makes for easier local testing. Not sure if there are any ramifications to the ember-cli-addon-tests?

Not an issue but I think you could write the tests in fastboot-tests? If it is more against the dummy app, it's fine too.

rondale-sc · 2017-08-07T17:08:50Z

This should be ready for another review. 🍻

rwjblue

Sorry for the delay here @rondale-sc.

Can you add a ember-lts-2-12 scenario to .travis.yml and config/ember-try.js?
Should we consider adding a blueprint that automatically puts {{head-layout}} to the application's application.hbs? This would be much easier than trying to do it automatically per-build (in a blueprint we can just use Blueprint.prototype.insertIntoFile)...

rwjblue · 2017-08-15T13:52:26Z

addon/templates/components/head-layout.hbs

@@ -1 +1,3 @@
-<meta name="ember-cli-head-start">{{head-content}}<meta name="ember-cli-head-end">
+{{#-in-element headElement}}


FYI - We have started taking steps to make this public API from the Glimmer VM perspective (see glimmerjs/glimmer-vm#619). It is unclear what Ember version that PR will correspond to, but it is a good step forward for our use case (and shows a future where we can drop the private - prefix).

rondale-sc · 2017-08-15T17:32:50Z

@rwjblue Added 2.12 to the ember-try config. I'm not sure where to begin with the blueprint idea.

rwjblue · 2017-08-24T14:42:19Z

RE: the blueprint I'm suggesting that we update the blueprint to use Blueprint.prototype.insertIntoFile to insert {{head-layout}}.

rondale-sc · 2017-08-24T14:43:31Z

@rwjblue That's an excellent idea. Will give that a try.

rondale-sc · 2017-08-28T15:26:25Z

I think this is ready to go now.

rwjblue · 2017-08-28T15:29:09Z

blueprints/ember-cli-head/index.js

+      fs.writeFileSync(fullPath, toWrite, { encoding: 'utf-8' });
+    } else {
+      let str = `You must add {{head-layout}} component to your topmost UI.
+This is usually your application.hbs, but was not found on your system.


Maybe use the full path (e.g. app/templates/application.hbs) in the message?

Updated! 🍻

rwjblue · 2017-08-28T15:29:26Z

blueprints/ember-cli-head/index.js

-  normalizeEntityName: function() { }
+  normalizeEntityName: function() { },
+  afterInstall() {
+    let fullPath = path.join(this.project.root, 'app/templates/applicati.hbs');


Typo here (should be app/templates/application.hbs)

rwjblue · 2017-08-29T14:03:15Z

LGTM, thanks for working through this with us!

@kratiahuja - Would you like to do a final review before landing and releasing?

kratiahuja · 2017-08-29T15:29:02Z

Sure I'll take a look at it in a while

kratiahuja

Looks good! Thanks @rondale-sc for the work here. I also think this needs a README update for folks needing to do the major version bump.

rondale-sc · 2017-08-31T13:49:14Z

@kratiahuja Thank you for reviewing. I added a section to the README and blueprint to help people moving from earlier versions. I wasn't sure what the version would be for this so I guessed and used 0.4.x for the README section title, but I'll update that once this is ready to go.

👍

rondale-sc · 2017-09-07T17:04:59Z

Is there anything else blocking this before we cut a release?

rwjblue · 2017-09-19T13:16:02Z

Thank you for all of your hard work here @rondale-sc!

Turbo87 · 2017-09-22T10:21:52Z

@rondale-sc @rwjblue am I interpreting correctly that this PR dropped support for Ember 2.4 and 2.8? might be worth it to mention that explicitly in the README and point to the v0.3.x releases for those Ember versions

rwjblue · 2017-09-22T11:42:16Z

@Turbo87 - Yep, you are correct. Happy to review and land a PR updating the README...

Turbo87 · 2017-09-22T11:48:38Z

Hmm I just noticed this in the README:

Take into account that version >= 0.3 of this addon require Ember 2.10+ and fastboot >=1.0.rc1 Please use 0.2.X if you don't fulfull both requirements.

Since 0.3.x was still tested against Ember 2.4 and 2.8 is that statement actually valid? I'm a little confused... 🤔

Use -in-element to render at appropriate time

51ac068

kratiahuja reviewed Aug 4, 2017

View reviewed changes

rondale-sc added 3 commits August 7, 2017 11:34

Fix fastboot tests

6efeccf

Remove support for 2.4 & 2.8

c3168d4

Remove ember-try scenario invocations from .travis.yml

d6402c0

rwjblue reviewed Aug 15, 2017

View reviewed changes

Update readme with instrunctions on how to use new style

1fc2703

rondale-sc force-pushed the use-glimmer-in-element branch from 2b24a6e to 1fc2703 Compare August 15, 2017 17:07

rwjblue reviewed Aug 28, 2017

View reviewed changes

rondale-sc force-pushed the use-glimmer-in-element branch from 5a5306a to 5295c57 Compare August 28, 2017 15:37

Add afterInstall to automatically add {{head-layout}} to application.hbs

9b1fad8

rondale-sc force-pushed the use-glimmer-in-element branch from 5295c57 to 9b1fad8 Compare August 29, 2017 13:46

rwjblue approved these changes Aug 29, 2017

View reviewed changes

kratiahuja approved these changes Aug 30, 2017

View reviewed changes

Add readme entry around upgrade to 0.4.x

5dc82a3

rondale-sc force-pushed the use-glimmer-in-element branch from 427b9de to 5dc82a3 Compare August 31, 2017 13:47

rwjblue merged commit 17b1e57 into ember-fastboot:master Sep 19, 2017

snewcomer mentioned this pull request Jun 21, 2018

update head to 0.4.1 ronco/ember-cli-meta-tags#71

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `-in-element` to render at appropriate time #37

Use `-in-element` to render at appropriate time #37

rondale-sc commented Aug 3, 2017 •

edited

kratiahuja left a comment

kratiahuja Aug 4, 2017

rondale-sc Aug 4, 2017

rwjblue Aug 4, 2017

kratiahuja Aug 4, 2017

rondale-sc commented Aug 7, 2017

rwjblue left a comment

rwjblue Aug 15, 2017

rondale-sc commented Aug 15, 2017

rwjblue commented Aug 24, 2017

rondale-sc commented Aug 24, 2017

rondale-sc commented Aug 28, 2017

rwjblue Aug 28, 2017

rondale-sc Aug 29, 2017

rwjblue Aug 28, 2017

rwjblue commented Aug 29, 2017

kratiahuja commented Aug 29, 2017

kratiahuja left a comment

rondale-sc commented Aug 31, 2017

rondale-sc commented Sep 7, 2017

rwjblue commented Sep 19, 2017

Turbo87 commented Sep 22, 2017

rwjblue commented Sep 22, 2017

Turbo87 commented Sep 22, 2017

		@@ -1 +1,3 @@
		<meta name="ember-cli-head-start">{{head-content}}<meta name="ember-cli-head-end">
		{{#-in-element headElement}}

Use -in-element to render at appropriate time #37

Use -in-element to render at appropriate time #37

Conversation

rondale-sc commented Aug 3, 2017 • edited

Details

Benefits

kratiahuja left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rondale-sc commented Aug 7, 2017

rwjblue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rondale-sc commented Aug 15, 2017

rwjblue commented Aug 24, 2017

rondale-sc commented Aug 24, 2017

rondale-sc commented Aug 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwjblue commented Aug 29, 2017

kratiahuja commented Aug 29, 2017

kratiahuja left a comment

Choose a reason for hiding this comment

rondale-sc commented Aug 31, 2017

rondale-sc commented Sep 7, 2017

rwjblue commented Sep 19, 2017

Turbo87 commented Sep 22, 2017

rwjblue commented Sep 22, 2017

Turbo87 commented Sep 22, 2017

Use `-in-element` to render at appropriate time #37

Use `-in-element` to render at appropriate time #37

rondale-sc commented Aug 3, 2017 •

edited