ember-fastboot / ember-cli-head

Ember Cli Addon for Adding Content to HTML Head
MIT License
98 stars 34 forks source link

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

Closed rondale-sc closed 6 years ago

rondale-sc commented 6 years ago

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:~


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.

screen shot 2017-08-03 at 2 23 56 pm

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

screen_shot_2017-08-03_at_2_39_14_pm

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:

screen_shot_2017-08-03_at_2_46_15_pm

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.

rondale-sc commented 6 years ago

This should be ready for another review. 🍻

rondale-sc commented 6 years ago

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

rwjblue commented 6 years ago

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

rondale-sc commented 6 years ago

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

rondale-sc commented 6 years ago

I think this is ready to go now.

rwjblue commented 6 years ago

LGTM, thanks for working through this with us!

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

kratiahuja commented 6 years ago

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

rondale-sc commented 6 years ago

@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 commented 6 years ago

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

rwjblue commented 6 years ago

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

Turbo87 commented 6 years ago

@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 commented 6 years ago

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

Turbo87 commented 6 years ago

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... 🤔