search

kimroen / ember-cli-document-title

Adding document title behaviour to your ember app
MIT License
215 stars 61 forks source link

Fix setting title in fastboot with glimmer2 #53

Closed kratiahuja closed 8 years ago

kratiahuja commented 8 years ago

49 tried to fix glimmer 2 but it wasn't correct.

Calling renderer._env.getDOM() in fastboot environments gives back undefined. Ember now allows you to get the DOM object (whether SimpleDOM in fastboot environment or DOM in browser).

This PR fixes setting title propertly in fastboot environment for apps using 2.9.beta.x. The earlier check was incorrect.

PS: I am keeping the else if (domForAppWithGlimmer2) since not all apps may be on 2.9.beta.x that will contain the document service.

cc: @chadhietala @kimroen @workmanw

Tested this with fastboot app.

workmanw commented 8 years ago

Thanks for fixing this. I haven't been using fastboot, so it didn't occur to me that this might be different.

kimroen commented 8 years ago

Thank you for this. I'm going to merge and release this now, but some changes are coming to this code in #38. Hopefully setting up tests (#52) first will make sure it doesn't break again.

kimroen commented 8 years ago

Released as ember-cli-document-title@0.3.3.

kratiahuja commented 8 years ago

Thank you @kimroen!

chriskrycho commented 8 years ago

This is actually a breaking change: I'm not sure when the service:-document private service got added, but it looks like it's not in 2.6 (which we're upgrading to currently), and as a result this change is breaking our app. 😬

We can of course target 0.3.2 in the meantime, but would you mind yanking 0.3.3 and republishing as 0.4.0? (I see you have other, semi-related changes coming in #38, but it won't fix this for us, or for anyone else who is using this add-on.)

Edit: notably, this is breaking our tests; we'll follow-up and confirm if it's breaking the app proper. It may be a week or so.

kimroen commented 8 years ago

This is actually a breaking change: I'm not sure when the service:-document private service got added, but it looks like it's not in 2.6 (which we're upgrading to currently), and as a result this change is breaking our app. 😬

Ouch, I'm really sorry about that. Is it breaking using FastBoot, or just running the app normally?

It could be that this happened because Glimmer didn't ship with 2.6 after all?

We can of course target 0.3.2 in the meantime, but would you mind yanking 0.3.3 and republishing as 0.4.0? (I see you have other, semi-related changes coming in #38, but it won't fix this for us, or for anyone else who is using this add-on.)

I don't mind at all, but I haven't yanked a release before so I'm not sure what the proper way is. Release 0.3.4 with something that works, and then release the change again as 0.4.0 and then mark 0.3.3 as a broken release?

Does anyone here know? I'll ask around.

chriskrycho commented 8 years ago

It’s breaking the app running normally, and 2.6 is relatively old at this point; we just hit it in the upgrade process as we’re trying to step up to 2.9 from 2.5.

I’m not sure what the process is, actually; I just know it’s possible. Thanks for getting back to me about it!

kimroen commented 8 years ago

Oh, 2.6! I totally read that as 2.9 - sorry again. I'll look into it.

kratiahuja commented 8 years ago

I believe service:-document exists on the Application Instance since Ember 2.6.x. Here is a twiddle that showcases that.

chriskrycho commented 8 years ago

Interesting. I’ll follow up when we’re able to come back to it and let you know what the deal is!

kimroen commented 8 years ago

I made an issue for this: https://github.com/kimroen/ember-cli-document-title/issues/54