Migrate away from ember-cli-head for <title> updates #168

raido · 2020-08-22T06:17:18Z

Use direct page title manipulation instead of ember-cli-head.

Fastboot apps have to remove <title> from app/index.html as before and for non-fastboot apps everything should work out of the box.

Closes #167

raido · 2020-08-22T06:19:37Z

tests/dummy/app/index.html

@@ -1,7 +1,7 @@
 <!DOCTYPE html>
 <html>
  <head>
-    <title>Initial page title - removed by addon initializer</title>
+    <!-- <title>Initial page title - removed by addon initializer</title> -->


Fastboot tests are green only if this is removed. But this is conflicting with the default app setup from ember new. It seems there is no access via service:-document to original title element to update it or remove it. This needs to be solved before this PR can move forward.

@rwjblue any ideas here?

I think apps will always have to remove this, we should remove it for them in our blueprint (making an ember-page-title blueprint). And in the RFC that suggests adding ember-page-title to all new apps we should explicitly state that this will be removed in favor of actually invoking {{title}} in the application.hbs.

FWIW, I'm pretty sure that this was already a constraint and not something new. Users of existing ember-page-title releases that are using ember-cli-fastboot along with ember-cli-head would have had to remove <title> from app/index.html.

Wait, how we can do this? If app/index.html has no <title> then search engines index pages without titles out of the box. All non fastboot apps that today set meaningful static title for search engines will have conflicting setup out of box.

I suppose we could have ember-cli-fastboot's blueprint remove it? But for the most part, isn't it safe to assume the search engine will run our JS nowadays?

Not sure about.

https://html.spec.whatwg.org/multipage/semantics.html#the-title-element - it does not say directly title is mandatory but seems strongly recommended.

https://validator.w3.org/ - from this tool perspective page without title is not valid HTML page.

I made tests assertions smarter to cope with 2 title tags being present for Fastboot tests with knowledge that in real life we'll ask people or make ember-cli-fastboot blueprint remove title from index.html.

tests/fastboot/title-test.js

addon/helpers/page-title.js

.travis.yml

README.md

rwjblue · 2020-08-24T19:16:03Z

addon/helpers/page-title.js

+    let childNodes = document.head.childNodes;
+    for (let i = 0; i < childNodes.length; i++) {
+      let node = childNodes.item(i);
+      if (node.tagName.toLowerCase() === 'title') {
+        document.head.removeChild(node);
+      }
+    }


Why do we need to remove the prior <title> tags that we find? In FastBoot the only thing that would be here that might include the prior <title> would have been someones head.hbs or something, the static <title> that is included in app/index.html would not be overridden by this.

It is not about the index.html static title tag, its about nested routes page-title invocations. We either lookup existing title element or clean up them all, so nested route titles like A | B | C - would render correctly.

What we can do is lookup existing title element we add on our own and remove all textNodes from it but end result is kind of the same.

Hmm, maybe we can track the current element and just update its contents on subsequent invocations?

https://github.com/ember-fastboot/simple-dom/blob/master/packages/%40simple-dom/document/src/node.ts#L38 - I have not found any update methods on simple-dom node implementation to do this.

I don't think we can track either, because if we imagine nested routes:

A -> B -> C ... n

Then each of them has page-title invocation in template, each of them triggers run.schedule afterRender queueing and the fact that the update method is separate method here, is only thing that helps runloop to run it just once, although N number of invocations tried to initiate it again before afterRender queue ran.

Not sure how we could keep track of the element if don't know which of the helper instances has latest element reference.

maybe we need to put the update logic into a service and the helper only updates some datastructure that keeps the order and builds out a string

This actually will not be a problem if we properly wait for transition to finish for all title updates.

addon/helpers/page-title.js

tests/acceptance/posts-test.js

rwjblue · 2020-08-24T19:28:29Z

tests/dummy/app/index.html

@@ -1,7 +1,7 @@
 <!DOCTYPE html>
 <html>
  <head>
-    <title>Initial page title - removed by addon initializer</title>
+    <!-- <title>Initial page title - removed by addon initializer</title> -->


I think apps will always have to remove this, we should remove it for them in our blueprint (making an ember-page-title blueprint). And in the RFC that suggests adding ember-page-title to all new apps we should explicitly state that this will be removed in favor of actually invoking {{title}} in the application.hbs.

rwjblue · 2020-08-24T19:29:36Z

tests/helpers/get-page-title.js

@@ -0,0 +1,7 @@
+// Testem appends progress to the title...


Ugh, that sucks 😭

tests/fastboot/title-test.js

rwjblue · 2020-08-25T13:05:09Z

Ready for a rebase now

raido · 2020-08-25T14:28:00Z

It is rebased and I will continue addressing PR feedback after CI run.

raido · 2020-08-25T16:24:09Z

Interesting, it seems by adding ember try scenario for fastboot and moving related dependencies out of package.json - made previously failing beta and canary scenarios pass again - https://travis-ci.org/github/adopted-ember-addons/ember-page-title/builds/721020282 🤔

raido · 2020-08-26T06:26:46Z

I removed ember-try fastboot scenario because the "typeof FastBoot" check allows us to run all other scenarios with browser + fastboot for more coverage.

beta and canary fails again which is good, because with previous ember-try scenario we would have missed that unless we duplicated all the scenarios for all default ones.

addon/services/page-title-list.js

raido · 2020-08-26T13:40:12Z

Beta and Canary failure seems to be coming from here:

https://github.com/glimmerjs/glimmer-vm/blob/6b0db3aaa9688c2906e0f2b06baed187e9991129/packages/%40glimmer/runtime/lib/dom/sanitized-values.ts#L43

Not sure how and why this happens though. Any ideas @rwjblue?

/var/folders/jp/2wh258pj23xg7sqgdf8g7gnc0000gn/T/broccoli-82322jRSxGVWj5u1V/out-183-append_ember_auto_import_analyzer/assets/vendor.js:54840
    var nodeURL = _nodeModule.module.require('url');
                                     ^

TypeError: Cannot read property 'require' of null
    at /var/folders/jp/2wh258pj23xg7sqgdf8g7gnc0000gn/T/broccoli-82322jRSxGVWj5u1V/out-183-append_ember_auto_import_analyzer/assets/@glimmer/runtime.js:288:1
    at internalRequire (/var/folders/jp/2wh258pj23xg7sqgdf8g7gnc0000gn/T/broccoli-82322jRSxGVWj5u1V/out-183-append_ember_auto_import_analyzer/assets/loader.js:54:1)
    at internalRequire (/var/folders/jp/2wh258pj23xg7sqgdf8g7gnc0000gn/T/broccoli-82322jRSxGVWj5u1V/out-183-append_ember_auto_import_analyzer/assets/loader.js:50:1)
    at internalRequire (/var/folders/jp/2wh258pj23xg7sqgdf8g7gnc0000gn/T/broccoli-82322jRSxGVWj5u1V/out-183-append_ember_auto_import_analyzer/assets/loader.js:50:1)
    at internalRequire (/var/folders/jp/2wh258pj23xg7sqgdf8g7gnc0000gn/T/broccoli-82322jRSxGVWj5u1V/out-183-append_ember_auto_import_analyzer/assets/loader.js:50:1)
    at require (/var/folders/jp/2wh258pj23xg7sqgdf8g7gnc0000gn/T/broccoli-82322jRSxGVWj5u1V/out-183-append_ember_auto_import_analyzer/assets/loader.js:87:1)
    at /var/folders/jp/2wh258pj23xg7sqgdf8g7gnc0000gn/T/broccoli-82322jRSxGVWj5u1V/out-183-append_ember_auto_import_analyzer/assets/vendor.js:72924:1
    at /var/folders/jp/2wh258pj23xg7sqgdf8g7gnc0000gn/T/broccoli-82322jRSxGVWj5u1V/out-183-append_ember_auto_import_analyzer/assets/vendor.js:72925:2
    at Script.runInContext (vm.js:131:20)

Seems there issue open over here: glimmerjs/glimmer-vm#1141

raido · 2020-08-26T16:13:23Z

The test / dummy app has few bugs still from previous times already. It's broken on Github pages current version as well, I'll see if it is an easy fix or evolves separate PR after this one.

raido · 2020-09-03T08:53:25Z

Canary is green now, beta bump is not available yet, therefore it's still failing.

rwjblue · 2020-09-03T17:03:07Z

Restarted the beta CI job

raido · 2020-09-07T11:38:56Z

@rwjblue any updates on review here?

raido · 2020-09-14T18:21:01Z

Any updates here?

addon/services/page-title-list.js

knownasilya

A couple questions, or potential changes

addon/services/page-title-list.js

tests/helpers/get-page-title.js

rwjblue · 2020-09-23T14:58:49Z

Thank you so much for pushing this to completion @raido!

raido commented Aug 22, 2020

View reviewed changes

tests/fastboot/title-test.js Outdated Show resolved Hide resolved

raido commented Aug 22, 2020

View reviewed changes

addon/helpers/page-title.js Outdated Show resolved Hide resolved

raido commented Aug 22, 2020

View reviewed changes

addon/helpers/page-title.js Outdated Show resolved Hide resolved

raido mentioned this pull request Aug 22, 2020

remove dependency on ember-cli-head #167

Closed

MelSumner requested a review from rwjblue August 24, 2020 19:02

rwjblue requested changes Aug 24, 2020

View reviewed changes

This was referenced Aug 25, 2020

Upgrade Ember.js and drop Node 8 #169

Closed

Upgrade to Ember v3.20, drop Node 8 #170

Merged

raido force-pushed the migrate-away-from-ember-cli-head branch from 522834b to 1de2674 Compare August 25, 2020 14:25

raido commented Aug 26, 2020

View reviewed changes

addon/services/page-title-list.js Outdated Show resolved Hide resolved

raido mentioned this pull request Aug 26, 2020

Error in FastBoot: "Cannot read property 'require' of null" glimmerjs/glimmer-vm#1141

Closed

knownasilya mentioned this pull request Sep 1, 2020

"You attempted to update length on Class" on ember canary #172

Closed

raido force-pushed the migrate-away-from-ember-cli-head branch from 1fc75be to 4746f01 Compare September 3, 2020 18:11

Migrate away from ember-cli-head for <title> updates

0bb7d33

raido force-pushed the migrate-away-from-ember-cli-head branch from 4746f01 to 0bb7d33 Compare September 3, 2020 18:22

raido marked this pull request as ready for review September 3, 2020 18:32

raido requested review from rwjblue and knownasilya September 3, 2020 18:32

rwjblue reviewed Sep 17, 2020

View reviewed changes

addon/services/page-title-list.js Outdated Show resolved Hide resolved

Clean up router.router fallback

04dc0a9

raido force-pushed the migrate-away-from-ember-cli-head branch from 1583326 to 04dc0a9 Compare September 18, 2020 13:48

raido requested a review from rwjblue September 18, 2020 13:52

knownasilya requested changes Sep 18, 2020

View reviewed changes

addon/services/page-title-list.js Show resolved Hide resolved

tests/helpers/get-page-title.js Show resolved Hide resolved

knownasilya approved these changes Sep 18, 2020

View reviewed changes

rwjblue approved these changes Sep 23, 2020

View reviewed changes

rwjblue added breaking this will be a breaking change, and a new major version should be released Feature labels Sep 23, 2020

knownasilya merged commit f8fff84 into ember-cli:master Sep 23, 2020

This was referenced Sep 23, 2020

Fix for #142 #143

Closed

Deprecation warning with Ember 3.20 #164

Closed

raido deleted the migrate-away-from-ember-cli-head branch September 23, 2020 17:54

This was referenced Sep 23, 2020

Failed acceptance tests #161

Closed

Blueprint should remove <title> tag from app/index.html if Fastboot is present #174

Closed

Clean up head.hbs being generated by blueprint #175

Merged

sbedeau mentioned this pull request Dec 2, 2020

[TECH] Mettre à jour ember-page-title sur Pix APP (PIX-1738) 1024pix/pix#2233

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate away from ember-cli-head for <title> updates #168

Migrate away from ember-cli-head for <title> updates #168

raido commented Aug 22, 2020 •

edited

Loading

raido Aug 22, 2020

knownasilya Aug 22, 2020

rwjblue Aug 24, 2020 •

edited

Loading

rwjblue Aug 24, 2020

raido Aug 24, 2020 •

edited

Loading

rwjblue Aug 25, 2020

raido Aug 25, 2020

raido Aug 26, 2020

rwjblue Aug 24, 2020

raido Aug 24, 2020

rwjblue Aug 24, 2020

raido Aug 24, 2020

raido Aug 24, 2020

knownasilya Aug 24, 2020

raido Aug 26, 2020

rwjblue Aug 24, 2020 •

edited

Loading

rwjblue Aug 24, 2020

rwjblue commented Aug 25, 2020

raido commented Aug 25, 2020

raido commented Aug 25, 2020 •

edited

Loading

raido commented Aug 26, 2020 •

edited

Loading

raido commented Aug 26, 2020 •

edited

Loading

raido commented Aug 26, 2020

raido commented Sep 3, 2020

rwjblue commented Sep 3, 2020

raido commented Sep 7, 2020

raido commented Sep 14, 2020

knownasilya left a comment •

edited

Loading

rwjblue commented Sep 23, 2020

Migrate away from ember-cli-head for <title> updates #168

Migrate away from ember-cli-head for <title> updates #168

Conversation

raido commented Aug 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwjblue Aug 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raido Aug 24, 2020 • edited Loading

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

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

Choose a reason for hiding this comment

rwjblue Aug 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwjblue commented Aug 25, 2020

raido commented Aug 25, 2020

raido commented Aug 25, 2020 • edited Loading

raido commented Aug 26, 2020 • edited Loading

raido commented Aug 26, 2020 • edited Loading

raido commented Aug 26, 2020

raido commented Sep 3, 2020

rwjblue commented Sep 3, 2020

raido commented Sep 7, 2020

raido commented Sep 14, 2020

knownasilya left a comment • edited Loading

Choose a reason for hiding this comment

rwjblue commented Sep 23, 2020

raido commented Aug 22, 2020 •

edited

Loading

rwjblue Aug 24, 2020 •

edited

Loading

raido Aug 24, 2020 •

edited

Loading

rwjblue Aug 24, 2020 •

edited

Loading

raido commented Aug 25, 2020 •

edited

Loading

raido commented Aug 26, 2020 •

edited

Loading

raido commented Aug 26, 2020 •

edited

Loading

knownasilya left a comment •

edited

Loading